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

Allow multiple CHEFS forms to be configured/displayed #1

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

kyle1morel
Copy link
Collaborator

Allow multiple CHEFS forms to be configured on the back end. Requests to the CHEFS API from the back end will create an Axios request with a dynamic basic auth by doing a lookup into the config map based on the form ID in the request. Submission data will be merged together for every form that is configured, before finally being filtered by user (if required) and sent to the front end.

Description

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

FE can pull from multiple data sources. BE has dynamic basic auth based on the requested form ID
@kyle1morel kyle1morel changed the title Allow multiple CHEFS forms to be configured Allow multiple CHEFS forms to be configured/displayed Nov 29, 2023
Copy link

Coverage Report (Application)

Totals Coverage
Statements: 51.82% ( 57 / 110 )
Methods: 38.46% ( 5 / 13 )
Lines: 69.84% ( 44 / 63 )
Branches: 23.53% ( 8 / 34 )

Copy link
Contributor

@wilwong89 wilwong89 left a comment

Choose a reason for hiding this comment

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

Provisionally approving, needs to fix failing tests.

Copy link
Member

@jujaga jujaga left a comment

Choose a reason for hiding this comment

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

Seems good for the most part - just a few minor odds and ends to address, including the failed linting/tests.

Comment on lines +17 to +23
"formId": "FORM_1_ID",
"formApiKey": "FORM_1_APIKEY"
},
"form2": {
"name": "FORM_2_NAME",
"formId": "FORM_2_ID",
"formApiKey": "FORM_2_APIKEY"
Copy link
Member

Choose a reason for hiding this comment

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

Style: Since we know the values are nested, might be able to call it just "id" and "apiKey" without needing the form prefix for the attribute names.

* BCeID/Business should only see their own submissions
*/
const filterData = (data: Array<ChefsSubmissionDataSource>) => {
const filterToUser = (req.currentUser?.tokenPayload as JwtPayload).identity_provider !== IdentityProvider.IDIR;
Copy link
Member

Choose a reason for hiding this comment

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

Just as a note - in the event we use a non-SSO Keycloak IAM we might not necessarily have the identity_provider attribute available. This is one of those few situations where I'm thinking it may be worth doing a defensive null check for tokenPayload?

app/src/services/chefs.ts Outdated Show resolved Hide resolved
Comment on lines 8 to 9
formId: string;
formApiKey: string;
Copy link
Member

Choose a reason for hiding this comment

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

Could be 'id' and 'apiKey' from earlier style comment if we wanted to be more terse.

frontend/src/router/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@TimCsaky TimCsaky left a comment

Choose a reason for hiding this comment

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

This is looking great!

Copy link

Coverage Report (Frontend)

Totals Coverage
Statements: 49.16% ( 176 / 358 )
Methods: 50.63% ( 40 / 79 )
Lines: 53.59% ( 112 / 209 )
Branches: 34.29% ( 24 / 70 )

@wilwong89 wilwong89 merged commit 8e57f3d into master Nov 30, 2023
18 checks passed
@wilwong89 wilwong89 deleted the feature/multi-form branch November 30, 2023 00:28
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.

5 participants