feat: Include VendorApprovalStatus table
#1328
Conversation
…ons.tests[].viewers[] and TestPlanReport.vendorReviewStatus
…endorApprovalStatus
|
#1329 should fix the build error happening here. |
# Conflicts: # client/tests/e2e/snapshots/saved/_data-management.html # client/tests/e2e/snapshots/saved/_test-queue.html
howard-e
changed the title
VendorApprovalStatus tableVendorApprovalStatus table
There was a problem hiding this comment.
This is a great improvement! I am especially happy about the removal of viewers. I didn't realize that this value wasn't being used to track viewed tests in normal runs as well.
I looked through the code and tested everything manually. Everything works great for the most part. I am a bit on the fence about the loss of information about reviewed tests for admins. It doesn't feel like a major problem but in the current version of the app when an admin views candidate reports they can see tests they have already looked at in the test nav. This doesn't seem too important but it does make me wander if we want that test nav to always show tests that have been viewed by the vendor reviewer since that is the most important info.
Other than that, just a few small comments inline. I'll pass back to you to address any that you think are appropriate
| @@ -295,12 +285,13 @@ const updatePhaseResolver = async ( | |||
| recommendedPhaseTargetDate: recommendedPhaseTargetDateValue, | |||
| deprecatedAt: null | |||
| }; | |||
| } else if (phase === 'RECOMMENDED') | |||
| } else if (phase === 'RECOMMENDED') { | |||
There was a problem hiding this comment.
Wild that these were missing.
| vendorId: user.vendorId || user.company?.id, | ||
| values: { | ||
| reviewStatus, | ||
| approvedAt: reviewStatus === 'APPROVED' ? new Date() : null |
There was a problem hiding this comment.
nit: does modification of this value need to be exposed outside of the service?
| testPlanReportId, | ||
| userId, | ||
| vendorId | ||
| }, |
There was a problem hiding this comment.
It might be that status sometimes applies to a plural or it might be that Ids is plural in the function name but it took a second for me to understand that this is the get single function and the below is the get all function. Maybe do a quick check to make sure all ids were passed in to prevent that confusion in the future?
| ) | ||
| .map(({ id }) => id); | ||
| const isApproved = testPlanReport.vendorReviewStatus === 'APPROVED'; | ||
| const date = new Date(); |
There was a problem hiding this comment.
Does this matter? It seems that there was no tracking of the approval date previously so I guess it is fine to use the current date but curious your thoughts
| { transaction } | ||
| ); | ||
|
|
||
| await queryInterface.createTable( |
There was a problem hiding this comment.
I appreciate the frozen table definition
| }); | ||
| } | ||
| } catch (error) { | ||
| // console.error('addViewerResolver.error', error); |
There was a problem hiding this comment.
Did you mean to leave this in? I wouldn't mind leaving uncommented error logging in
Address #1310