-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat(mrf-admin-view): hawkeye v1.0 #7965
base: develop
Are you sure you want to change the base?
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 2150 Passed, 1 Skipped, 5m 6.71s Total duration (1m 39.78s time saved) |
|
342a9e8
to
73eaece
Compare
shared/types/submission.ts
Outdated
|
||
const SubmittedNonApprovalStep = z.object({ | ||
isApproval: z.literal(false), | ||
submittedAt: z.string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does zod have a timestamp type for validation? if so, maybe we can use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - fixed referring to zod's datetime documentation with submittedAt: z.string().datetime({ precision: 3 })
db66d56
to
b492151
Compare
@@ -0,0 +1,26 @@ | |||
import { WorkflowStatus } from '~shared/types' | |||
|
|||
/** Gets the business friendly string for current step of workflow. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are helper functions to generate the view for mrf submissions responses
frontend/vite.config.ts
Outdated
define: { | ||
// On local dev, global is undefined, causing decryption workers to fail. | ||
// This is a workaround based on https://github.com/vitejs/vite/discussions/5912. | ||
global: 'globalThis', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used the bypass an error faced as described by the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with removing the whole global access chunk in decryption.worker.ts and it seemed to have resolved the csv generation issue on local.
Might want to look into that solution since I would prefer deleting code than to have more config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the global reference + vite config, tested and it works well!
/** | ||
* Builds the metadata for a multirespondent form submission. | ||
*/ | ||
export const buildMrfMetadata = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mrfMetadata refers to data which is not stored but derived from stored data when buildMrfMetadata
is executed. This metadata is used in the submission and submissionMetadata requests to display metadata info about the submission in the responses table, downloaded csv file and individual submission response detail page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested to generate the correct metadata given the submission data
@@ -112,7 +112,7 @@ export class EncryptedResponseCsvGenerator extends CsvGenerator { | |||
if (this.hasBeenProcessed) return | |||
|
|||
// Create a header row in CSV using the fieldIdToQuestion map. | |||
const headers = this.isMrf ? MRF_CSV_HEADERS : NON_MRF_CSV_HEADERS | |||
const headers = this.isMrf ? [...MRF_CSV_HEADERS] : [...NON_MRF_CSV_HEADERS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The de-structuring is necessary as we are mutating the headers
array below. De-structuring creates a new array which prevents mutations from affecting the referenced const array (ie, MRF_CSV_HEADERS
or NON_MRF_CSV_HEADERS
).
Otherwise, headers
points to the MRF_CSV_HEADERS
or NON_MRF_CSV_HEADERS
const array and mutates the same memory address which is shared across multiple runs of process()
. This causes issues in cases where the memory is shared/reused,specifically test cases, which causes the headers to contain the headers from the previous runs of process()
and for the test cases to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good explanation to highlight that we need to create new array as we are mutating it at a later stage. Would recommend to put a short note in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a NOTE and TODO
fae67dc
to
c60e95a
Compare
c60e95a
to
75ac070
Compare
@@ -193,7 +194,8 @@ const MRF_RESPONSE_TABLE_COLUMNS: Column<ResponseColumnData>[] = [ | |||
Header: MRF_FIRST_STEP_TIMESTAMP_LABEL, | |||
accessor: 'submissionTime', | |||
width: 250, | |||
disableResizing: true | |||
minWidth: 250, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: re-using the same fix as existing storage mode responses table columns to include minWidth which allows the bar to expand for the rightmost column when the screen is small and horizontal scroll is enabled.
448f2d0
to
2384693
Compare
...min-form/responses/ResponsesPage/storage/UnlockedResponses/ResponsesTable/ResponsesTable.tsx
Outdated
Show resolved
Hide resolved
...min-form/responses/ResponsesPage/storage/UnlockedResponses/ResponsesTable/ResponsesTable.tsx
Show resolved
Hide resolved
return MRF_STATUS.COMPLETED | ||
case WorkflowStatus.PENDING: | ||
return MRF_STATUS.PENDING | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/nit since it is likely that all the WorkflowStatus
is fully defined, we would want the typesystem to let us know that we need to update the cases here rather than it falling through to default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made it more explicit for undefined
to become ''
and explicitly specified return literals of the function.
...s/ResponsesPage/storage/utils/EncryptedResponseCsvGenerator/EncryptedResponseCsvGenerator.ts
Show resolved
Hide resolved
src/app/modules/submission/multirespondent-submission/multirespondent-submission.service.ts
Show resolved
Hide resolved
switch (workflowStatus) { | ||
case WorkflowStatus.COMPLETED: | ||
case WorkflowStatus.APPROVED: | ||
case WorkflowStatus.REJECTED: | ||
return MRF_STATUS.COMPLETED | ||
case WorkflowStatus.PENDING: | ||
return MRF_STATUS.PENDING | ||
default: | ||
case undefined: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Should still have a default case for fall-through, recommend to utilise exhaustive switch
- Check for undefined should be outside of switch case. Could be something before line 21.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments on exhaustive switch in frontend/src/features/admin-form/responses/common/utils/mrfSubmissionView.ts
. That should be the last item before we're good to go!
Problem
Currently, admins want a way to track the status of their multi-respondent submissions/workflows to see:
Closes FRM-1919
Solution
Add the above-mentioned information to the responses dashboard, csv download and individual responses page so admins can track the status.
Breaking Changes
Before & After Screenshots
BEFORE:
AFTER:
New columns added and new name for timestamp column.
Tests
TC1: Hawk eye view dashboard shows the new
status
,current step
andsubmission time of first step
TC2: Reject and approve show correctly.
No
)Yes
)TC3: Storage mode and storage mode with with payments unaffected.
status
,current step
andsubmission time of first step
do not exist in the dashboard and the other expected columns exist (ie, same as before the changes introduced in this PR / refer to prod env)status
,current step
andsubmission time of first step
do not exist in the csv.status
,current step
andsubmission time of first step
do not exist and the dashboard is same as previous (ie, same as before the changes introduced in this PR / refer to prod env).