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

Aps 1757 raise error if characteristics are different #2739

Merged

Conversation

danhumphreys-moj
Copy link
Contributor

@danhumphreys-moj danhumphreys-moj commented Dec 20, 2024

Ref: APS-1757

Raise error if characteristics aren't consistent between beds when beds are in the same room.

Other issues resolved such as:

  • construct questionCriteriaMapping when the seed job is executed e.g. in processDataFrame, and then 'pass it around' to functions that need it (ref)

  • questionCriteriaMapping shouldn’t be a class instance variable to avoid concurrency issues (ref)

  • introduce a fun buildRoomCode(qCode,roomNumber) = "$qCode - $roomNumber" to reduce duplication (ref)

  • consider using something like this to aid readability: rows.addCharacteristics("Is this room located on the ground floor?", listOf("N","Y","Y")) (ref)

  • copy all questions into a List in this class as an instance variable, and then refer to that here instead of questionCriteriaMapping.questionToCharacterEntityMapping.keys (ref)

  • consider building bespoke data classes rather than using existing entity classes - this should help simplify seed job logic (see approach in seed premises job)

@danhumphreys-moj danhumphreys-moj changed the base branch from main to bug/fix-multiple-beds-in-room-issue-with-Excel-seed-job December 20, 2024 15:39
@danhumphreys-moj danhumphreys-moj force-pushed the bug/fix-multiple-beds-in-room-issue-with-Excel-seed-job branch from 19cc622 to 5dc6481 Compare December 20, 2024 16:24
Base automatically changed from bug/fix-multiple-beds-in-room-issue-with-Excel-seed-job to main December 20, 2024 16:34
@danhumphreys-moj danhumphreys-moj force-pushed the APS-1757-raise-error-if-characteristics-are-different branch 5 times, most recently from e374b51 to a016fe8 Compare January 8, 2025 10:02
@danhumphreys-moj danhumphreys-moj marked this pull request as ready for review January 8, 2025 13:45
@danhumphreys-moj danhumphreys-moj force-pushed the APS-1757-raise-error-if-characteristics-are-different branch from a016fe8 to 0697a9f Compare January 8, 2025 13:47
Copy link
Contributor

@davidatkinsuk davidatkinsuk left a comment

Choose a reason for hiding this comment

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

A really great set of improvements. LGTM 👍

@danhumphreys-moj danhumphreys-moj force-pushed the APS-1757-raise-error-if-characteristics-are-different branch from 0697a9f to eb1da94 Compare January 9, 2025 10:37
@danhumphreys-moj danhumphreys-moj force-pushed the APS-1757-raise-error-if-characteristics-are-different branch from eb1da94 to a81b96f Compare January 9, 2025 10:47
@danhumphreys-moj danhumphreys-moj merged commit 8461768 into main Jan 9, 2025
7 checks passed
@danhumphreys-moj danhumphreys-moj deleted the APS-1757-raise-error-if-characteristics-are-different branch January 9, 2025 10:56
@danhumphreys-moj
Copy link
Contributor Author

Thanks @davidatkinsuk and appreciate your help with this. I know this refactoring wasn't entirely necessary but having your equivalent seed premises job along with your comments/feedback has been a really useful exercise for me.

The refactoring work done in the penultimate commit really meant I was able to address the ticket requirements in a much more straightforward manner.

Appreciate the opportunity to have done this. @Rosie-Brigham revisiting previous work and learning from it was something we raised during my last 121 and I wanted to do. This is a good example.

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