Skip to content

Make supplier and other array params mandatory - #476

Open
bryophyta wants to merge 2 commits into
mainfrom
pf/array-search-params-mandatory
Open

Make supplier and other array params mandatory#476
bryophyta wants to merge 2 commits into
mainfrom
pf/array-search-params-mandatory

Conversation

@bryophyta

Copy link
Copy Markdown
Contributor

What does this change?

We have a number of search parameters in the UI that take arrays, which are currently optional. There's no clear meaning in this context for 'undefined', as opposed to an empty array, though, and we almost always expected the values to be arrays (empty or otherwise), so it makes sense to make sure it's always an array, so that the code is simpler (fewer checks to make sure it's an array) and easier to reason about.

How to test

  • Run unit tests -- do they pass?
  • Make some searches using the relevant params via the URL and the UI -- are there any regressions?

Images

Introducing DB migrations?

@bryophyta
bryophyta requested a review from a team as a code owner September 22, 2025 08:15
@github-actions

github-actions Bot commented Sep 22, 2025

Copy link
Copy Markdown

@bryophyta
bryophyta changed the base branch from main to pf/separate-layout-component-for-ticker September 22, 2025 10:42
@bryophyta
bryophyta force-pushed the pf/array-search-params-mandatory branch from ecd8021 to 3edee35 Compare September 22, 2025 10:45
@bryophyta
bryophyta force-pushed the pf/separate-layout-component-for-ticker branch from 4665893 to 4984511 Compare September 22, 2025 15:20
@bryophyta
bryophyta force-pushed the pf/array-search-params-mandatory branch from 3edee35 to cae3791 Compare September 22, 2025 15:23

@lindseydew lindseydew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, this is looking like a good shout 🙌

Just had a question about the test set up

expect(config).toEqual({
view: 'feed',
query: {
...defaultQuery,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How come ...defaultQuery is being passed in twice here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks! just a copy-paste error I think 🤦 I've fixed it now 👍

Base automatically changed from pf/separate-layout-component-for-ticker to main September 23, 2025 07:43
@bryophyta bryophyta added the maintenance Departmental tracking: maintenance work, not a fix or a feature label Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Departmental tracking: maintenance work, not a fix or a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants