Skip to content

Conversation

@mcgarrye
Copy link
Collaborator

@mcgarrye mcgarrye commented Dec 22, 2025

This PR addresses #(5562)

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

Updates MSQ GET list query params to allow for: pagination, search by name, filtering by (applicationSection, jurisdiction, status), and ordering by (applicationSection, jurisdiction, name, updatedAt).

How Can This Be Tested/Reviewed?

Start application
Using api, test each of the new query param options. You will need to hit it using curl or a API tool such as Postman.
Examples:
curl -X 'GET' \ 'http://localhost:3100/multiselectQuestions?page=1&limit=10' \ -H 'accept: application/json'

curl -X 'GET' \ 'http://localhost:3100/multiselectQuestions?filter%5B0%5D%5B%24comparison%5D=IN&filter%5B0%5D%5Bstatus%5D=active,visible' \ -H 'accept: application/json'

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@netlify
Copy link

netlify bot commented Dec 22, 2025

Deploy Preview for bloom-flagly canceled.

Name Link
🔨 Latest commit 55e6aab
🔍 Latest deploy log https://app.netlify.com/projects/bloom-flagly/deploys/694ad03ae0d5e700088388e7

@netlify
Copy link

netlify bot commented Dec 22, 2025

Deploy Preview for bloom-angelopolis canceled.

Name Link
🔨 Latest commit 55e6aab
🔍 Latest deploy log https://app.netlify.com/projects/bloom-angelopolis/deploys/694ad03afe3ec10008d07729

@netlify
Copy link

netlify bot commented Dec 22, 2025

Deploy Preview for partners-bloom-dev ready!

Name Link
🔨 Latest commit 55e6aab
🔍 Latest deploy log https://app.netlify.com/projects/partners-bloom-dev/deploys/694ad03a440e5c0008264658
😎 Deploy Preview https://deploy-preview-5711--partners-bloom-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@mcgarrye mcgarrye added the 1 review needed Requires 1 more review before ready to merge label Dec 22, 2025
Copy link
Collaborator

@ludtkemorgan ludtkemorgan left a comment

Choose a reason for hiding this comment

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

I was only able to test this by switching the endpoint to a POST endpoint instead of a GET. I struggled to test these filters while the endpoint is a GET. The swagger UI does not handle it correctly and trying to use postman or any other tool gives me errors due to incorrect formatting of the filter.

What would be your thoughts on making a new POST list endpoint and then slowly migrate all the current usages of the list endpoint to the POST one? You can still use the same service code, just add a new endpoint @Post('list') and still call this.multiselectQuestionService.list. I'm ok with sticking with a GET though if you prefer to try to use GETs for these use cases

Also I think some small updates to the e2e test would make our test coverage even better! But I was unable to find any functional issues!

expect(multiselectQuestions).toContain(multiselectQuestionB.text);
});

it('should get multiselect questions from list endpoint when params are sent', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: The unit tests are good tests for making sure we are creating the right queries with the passed in parameters. However they don't truly test if the correct data is returned from the DB since we are mocking out the DB responses. I like the tests here but we could get some extra verification by updating what this test tests.
Some suggestions:

  • create some multiselectQuestions that should not be expected to be returned
  • Add more than just a filter on jurisdiction to make sure multiple filters work
  • Create a new jurisdiction just for this test so we can verify an exact length instead of toBeGreaterThanOrEqual. This would work because we are filtering by jurisdiction id and these would be the only MSQs that are tied to that jurisdiction
  • Add an orderBy to make sure response it returned in the correct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not going to be able to add all these tests before I sign off. Will add them in the new year

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 review needed Requires 1 more review before ready to merge questions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants