-
Notifications
You must be signed in to change notification settings - Fork 212
Prevent overwriting changes to prefilled data in model catalog register form #3921
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
Prevent overwriting changes to prefilled data in model catalog register form #3921
Conversation
8a401d1
to
63de336
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3921 +/- ##
==========================================
- Coverage 84.47% 84.47% -0.01%
==========================================
Files 1580 1580
Lines 37566 37568 +2
Branches 10592 10592
==========================================
Hits 31734 31734
- Misses 5832 5834 +2 see 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…er form Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
…fix" This reverts commit 9591539.
Signed-off-by: Mike Turley <[email protected]>
08ce77f
to
9cbf8b1
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mturley, YuliaKrimerman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3e8b8b2
into
opendatahub-io:main
Resolves https://issues.redhat.com/browse/RHOAIENG-21573
Description
This is a hacky fix, a better fix would be to refactor this page so it fetches catalog data before mounting its state hook and avoids prefilling in a useEffect. Opened a followup issue for that refactor: https://issues.redhat.com/browse/RHOAIENG-21678
How Has This Been Tested?
Test Impact
I attempted to write a Cypress test for this and ran into issues. This fix is low-risk and easy to verify - following up to add a test is covered in the tech debt issue https://issues.redhat.com/browse/RHOAIENG-21678. See that issue's description for context on the trouble I had.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main