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

FS-4446: Consistent CSV File Columns #500

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Conversation

suegarner
Copy link
Contributor

@suegarner suegarner commented Sep 23, 2024

FS-446

Change description

Rearranged the default order of the columns so that Score comes directly after Date submitted regardless whether there is a score entered or not. Previously Score and Score Subcriteria could be swapped depending on if the first row had been scored yet)

  • Unit tests and other appropriate tests added or updated
  • README and other documentation has been updated / added (if needed)
  • Commit messages are meaningful and follow good commit message guidelines (e.g. "FS-XXXX: Add margin to nav items preventing overlapping of logo")

How to test

Open OUTPUT_TRACKER report for a window that has applications but no scores. Open OUTPUT_TRACKER for a window that has applications and are all scored. The columns should be in the same order for both reports.

Screenshots of UI changes (if applicable)

Before fix (COF OUTPUT_TRACKER report):
image
image

NSTF OUTPUT_TRACKER report:
image
image

Copy link

@sfount sfount left a comment

Choose a reason for hiding this comment

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

Looks good, a subtle difference but looks like one that could crop up for users at anytime!

The description on the PR is really helpful context on why that order might have been shifted, if that could be added to the commit description itself that should help any future devs outside of GitHub looking at why that might have changed.

db/queries/assessment_records/queries.py Show resolved Hide resolved
db/queries/assessment_records/queries.py Show resolved Hide resolved
Copy link

@sfount sfount left a comment

Choose a reason for hiding this comment

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

PR looks really good and well covered! One small suggestion for making the coverage more dynamic but I think this is good to undraft and review/ approve. Great fix!

tests/test_db_function.py Outdated Show resolved Hide resolved
Rearranged the default order of the columns so that Score comes directly after Date submitted regardless whether there is a score entered or not. Previously Score and Score Subcriteria could be swapped depending on if the first row had been scored yet)
Changed so that the report remains in the same order regardless if applications have been scored or not
For the OUTPUT_TRACKER report, the test checks the column order in a report of unscored applications against the column order in a report of scored applications. The scored report will have more columns so we're comparing all the columns in the unscored report against the first x columns in scored report.
@suegarner suegarner marked this pull request as ready for review September 26, 2024 12:35
Copy link
Contributor

@srh-sloan srh-sloan left a comment

Choose a reason for hiding this comment

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

Well done for tracking this down, can't have been fun! Solution looks good, and a nice test as well 👌

Copy link
Contributor

@nuwan-samarasinghe nuwan-samarasinghe left a comment

Choose a reason for hiding this comment

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

LGTM

@suegarner suegarner merged commit 33c2c99 into main Sep 27, 2024
14 checks passed
@suegarner suegarner deleted the FS-4446-ConsistentCSVColumns branch September 27, 2024 15:03
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.

4 participants