Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with missing region name #131

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Conversation

cjrace
Copy link
Collaborator

@cjrace cjrace commented Aug 20, 2024

Brief overview of changes

Fixed a bug that happened in files with only one region column present and no region rows.

Why are these changes being made?

The app currently will error and then crash with no feedback to users, we want to do better than that and always give feedback on failing files.

Example when running the app locally on the main branch with the test files (same error in the prod logs)
image

Detailed description of changes

  • Added a test file that errors on the production site currently.
  • Added a unit test to check there is no error when screening that file (this failed until I added the fix, and now it passes)
  • Updated the region_combinations() test to only execute if both region_name and region_code columns are present
  • Updated the country_combinations() test to remove the check for country_code being present as no file can get to the main checks stage without it

Additional information for reviewers

The test I've fixed - region_combinations() doesn't need to fail if only one region column is present as another test region_col_present() checks for this already.

To make it easy to test, you can use the test data files I've added in this PR, try screening them against the current prod version - https://rsconnect/rsc/dfe-published-data-qa/, and then try doing the same on the dev environment (where this has deployed to after I opened the PR - https://rsconnect-pp/rsc/dev-dfe-published-data-qa/). You should see the app on prod will crash but it'll handle it neatly on the dev environment.

Issue ticket number/s and link

No related issues

cjrace added 2 commits August 20, 2024 18:07
…ing, and then remove unnecessary condition for country combinations
@cjrace cjrace requested review from rmbielby and jen-machin August 20, 2024 18:04
@cjrace cjrace marked this pull request as ready for review August 20, 2024 18:09
@cjrace cjrace changed the base branch from main to development August 20, 2024 18:09
@cjrace cjrace changed the title Bug missing region name Fix bug with missing region name Aug 20, 2024
@cjrace
Copy link
Collaborator Author

cjrace commented Aug 20, 2024

Copy link

@jen-machin jen-machin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works as expected on the dev version, definitely less frustrating than the old version!

@cjrace cjrace merged commit 362c8ba into development Aug 21, 2024
7 checks passed
@cjrace cjrace deleted the bug-missing-region-name branch August 21, 2024 07:32
@cjrace cjrace mentioned this pull request Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants