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

Remove incomplete warnings on submission summary page #2387

Merged
merged 14 commits into from
Apr 30, 2024

Conversation

pearl-truss
Copy link
Contributor

@pearl-truss pearl-truss commented Apr 26, 2024

Summary

  • If there's missing data the field is not included in the UI for the submission summary page
  • submission date is now included for all submitted submissions on the submission summary page

Related issues

https://jiraent.cms.gov/browse/MCR-4072
MCR-4069
https://jiraent.cms.gov/browse/MCR-4071

Test cases covered

  • Missing fields are not displayed on submission summary page
  • Warning for multiple files is not included on any pages (Hana note: think this is the implied scope of MCR-4071 and trying to prepare for linked rates)
  • renders SHARED tag on docs table to CMS users when packages across submissions present
  • renders SHARED tag to state users when packages across submissions present

QA Guidance

  • It's not possible to get a new submission into a state with missing required information. Check test

@haworku
Copy link
Contributor

haworku commented Apr 29, 2024

👋 Could we please handle this explainMissingData stuff the same way on the different summary pages.

I'm seeing my recent work brought in const isSubmittedOrCMSUser = contract.status === 'SUBMITTED' || loggedInUser?.role === 'CMS_USER' to adjust the submitted logic (which I realized is not just about contract status but also functionally about what user is viewing what). I should have done that everywhere. Your approach is a little different and also works.

Can you pick one approach and extend to both summary pages - there's not any divergence? I don't want us to have different implementations of feeding logic to explainMissingData and have to maintain multiple paths.

Copy link
Contributor

@haworku haworku left a comment

Choose a reason for hiding this comment

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

Approving to allow merge -- but left a comment about something I would like to see adjusted at some point

Copy link
Contributor

@macrael macrael left a comment

Choose a reason for hiding this comment

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

These changes make sense to me, I agree with Hana about trying to make these checks consistent

@haworku
Copy link
Contributor

haworku commented Apr 29, 2024

I had to add tests for SHARED tag because the refactored fix to UploadedDocumentsTable for the missing data messages also would impact SHARED tag behavior.

I intentionally chose to move toward a domain agnostic prop hideDynamicFeedback on the UploadedDocumentsTable. I think this is like the third or fourth change on props for that table in the last couple months. Is it clear?

My reasoning -- Part of what I think caused regressions here is confusion about the intent of something like a isSubmitted or isEditable or isEditing prop. As different developers came through and changed the component for different features and use cases, we were interpreting those props differently. - e.g. something could be submitted once but unlocked, it could be unlocked and editable but maybe not for a specific user type, etc).

Seems like ultimately what we needed a prop for is "do we conditionally show user feedback about validations and allow edit buttons, thus treating these documents as dynamic content that could be soon changed ... OR do we display this as read only historic data that is fine with its imperfections".

@haworku haworku requested a review from macrael April 29, 2024 22:38
@haworku haworku merged commit 0b95fea into main Apr 30, 2024
28 checks passed
@haworku haworku deleted the mcr-4072-incomplete-warnings-on-submission-summary-page branch April 30, 2024 13:38
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.

3 participants