Conversation
There was a problem hiding this comment.
Hey @sachaudh - I've reviewed your changes - here's some feedback:
- Ensure you still handle the legacy
PARTIAL_ERRORrunState (or provide a migration) so existing reports don’t break until the API moves to the two new states. - In
PartialReportModal, when used for email (noonDownload), consider adding an explicit “OK” or close button in the modal actions so users have a clear way to dismiss it.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at 32a0ca5. To use with deploy scripts, first |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15449 +/- ##
==========================================
- Coverage 49.24% 49.23% -0.02%
==========================================
Files 2578 2578
Lines 189182 189182
==========================================
- Hits 93161 93142 -19
- Misses 88686 88700 +14
- Partials 7335 7340 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dvail
left a comment
There was a problem hiding this comment.
LGTM. Regarding the bot comments:
Ensure you still handle the legacy PARTIAL_ERROR runState (or provide a migration) so existing reports don’t break until the API moves to the two new states.
This is a good catch, but we only need to take action if this wasn't done on the BE. Do you know if the API has changed so that PARTIAL_ERROR will never be a valid response to the UI?
In PartialReportModal, when used for email (no onDownload), consider adding an explicit “OK” or close button in the modal actions so users have a clear way to dismiss it.
This is a good idea too, it would be nice to have an explicit "Close" button in these modals for clarity. I see the download modals use "Cancel" - would "Close" be appropriate in both cases?
I checked the PF design guidelines and I couldn't find any example of a modal that didn't have a confirmation and a cancellation button 🤔 , although I'm sure this pattern is something we have replicated throughout the UI.
Ah, yeah nice catch. In my opinion, it makes sense to have Looking at the code, I see some places we do show |
|
I'm going to be smart about the change. I have two PRs branching off this one. I need to make a change to the last one anyways, so i'm going to make the fix there. That way I can just merge this and not go through a full CI run again. |
Description
This PR makes some adjustments to the partial reporting feature. We've split
PARTIAL_ERRORintoPARTIAL_SCAN_ERROR_DOWNLOADandPARTIAL_SCAN_ERROR_EMAIL.Note: I'll add a follow-up PR to refactor the labels we use for the report jobs according to what Mansur recommended in the last UI/UX meeting. Attached a screenshot of what we intend to change it to:
Screenshots
User-facing documentation
Testing and quality
Automated testing
How I validated my change