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.

Sign up for GitHub

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

Jump to bottom

APS-1702 Suitability search filter changes #2274

Merged
merged 1 commit into from
Jan 14, 2025
Merged

APS-1702 Suitability search filter changes #2274

merged 1 commit into from
Jan 14, 2025

Conversation

bobmeredith
Copy link
Contributor

@bobmeredith bobmeredith commented Jan 6, 2025 •
edited
Loading

Context

https://dsdmoj.atlassian.net/browse/APS-1702

Changes in this PR

Update to the suitability search filters.

  • Removes start date input.
  • Slight layout change.
  • Removes 'gender' input
  • Renames some filter criteria. In order to avoid affecting other parts of the system and more creeping changes elsewhere, this renaming and re-ordering is local to the suitability search.
  • 'Catered' is moved from a room-level to an Ap level criterion - although it doesn't become a member of the 'Risks and offences' grouping an assess where the other AP level criteria are.
  • 'Hate crimes' and AP level arson removed from the search page only.
  • I've not implemented a 'clear all' as it was not mentioned in the ticket and after discussion with the designers makes little sense. A 'Reset to defaults' i.e. restore to assessment values probably makes more sense.

Screenshots of UI changes

Before
After

@bobmeredith bobmeredith force-pushed the feature/APS-1702_Suitability_search_filter_changes branch from 63e949e to 27ade91 Compare January 7, 2025 10:07
@bobmeredith bobmeredith marked this pull request as ready for review January 7, 2025 10:18
@bobmeredith bobmeredith force-pushed the feature/APS-1702_Suitability_search_filter_changes branch from 27ade91 to 811d0ae Compare January 7, 2025 10:21
froddd
froddd reviewed Jan 7, 2025
View reviewed changes
server/utils/utils.ts Outdated
Comment on lines 67 to 70
export const lowerCase = (string: string) => {
const result = Case.lower(string)
return result
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Introducing a variable here seems unnecessary -- though this method seems unnecessary in itself... 🤷‍♂️

Copy link
Contributor Author

@bobmeredith bobmeredith Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yup - nice spot. I think I added a log in there.
Turns out that this method messes up hyphens.
Anyway - I'll revert this.

@bobmeredith bobmeredith force-pushed the feature/APS-1702_Suitability_search_filter_changes branch 4 times, most recently from fe06e13 to a0d8530 Compare January 14, 2025 11:09
froddd
froddd approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@froddd froddd 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 to go, but if possible some more tidying up around related services and factories would be good to see to truly represent latest state of play.

server/testutils/factories/spaceSearchParameters.ts Outdated Show resolved Hide resolved
server/testutils/factories/spaceSearchParameters.ts Outdated Show resolved Hide resolved
server/utils/placementCriteriaUtils.ts Outdated
Comment on lines 100 to 108
const filterByTypeOrder = (mapping: CriteriaLabelMap, defaultLabels: CriteriaLabelMap): CriteriaLabelMap =>
Object.entries(mapping).reduce(
(criteria, [key, value]) => ({ ...criteria, [key]: value || defaultLabels[key] }),
{} as CriteriaLabelMap,
)

export const spaceSearchCriteriaApLevelLabels = filterByTypeOrder(apLevelSearchCriteria, placementCriteriaLabels)

export const spaceSearchCriteriaRoomLevelLabels = filterByTypeOrder(roomLevelSearchCriteria, placementCriteriaLabels)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to see a test or 2 around these, if only to document what they're meant to achieve.

Alternatively, those could be set rather than processed -- this is a fair bit of logic for a couple of default labels!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. The idea was to avoid replicating all the characteristic labels for the suitability search, and just replacing the altered ones. As it happens, most are being overridden so you're probably right and I should just re-declare the entire set.

server/utils/match/index.test.ts Show resolved Hide resolved
@bobmeredith bobmeredith force-pushed the feature/APS-1702_Suitability_search_filter_changes branch from 4cb4848 to b8d8df4 Compare January 14, 2025 17:07
@bobmeredith bobmeredith merged commit 8475fb9 into main Jan 14, 2025
7 checks passed
@bobmeredith bobmeredith deleted the feature/APS-1702_Suitability_search_filter_changes branch January 14, 2025 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@froddd froddd froddd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobmeredith @froddd