-
Notifications
You must be signed in to change notification settings - Fork 256
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
Clear list of errors before saving project form #10749
Clear list of errors before saving project form #10749
Conversation
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.
Noticed the issue is targeted at 2.9.next1, rather than 2.9.0
deabf74
to
08e3c9a
Compare
08e3c9a
to
e13ea8d
Compare
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.
LGTM
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.
The change looks good, but process wise there's some issues
- remember this can't merge to
master
as it's targeted at 2.9.next1 - the checkbox
The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
hasn't been followed. The original issue is marked as dev automation and there are no automated tests. That can be changed to manual test, however this feels like something automated tests would be good for. With automated test it might also be able to ship in 2.9.0
Sure thing, we can write some e2e tests to cover this one.. I'll mark this as draft for the time being. |
@rak-phillip I wasn't aware we had the 2.9.0 milestone available. Hence, I tagged the issue with v2.9-Next1. Since the backend changes for limit validations are going in 2.9.0, can we have this merged into 2.9.0 as well? |
@richard-cox I wrote some e2e tests to cover the change in behavior. Let me know if you'd like to see anything else. With e2e tests in place, and if we deem them sufficient, do we have any other blockers that would prevent this from merging for v2.9.0? |
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.
The POs aren't well documented, but very roughly if there's a vue component there probably should be a matching PO which should contain all the interaction and info gathering that the e2e specs need. This avoids a lot of repeated code and tests are generally less brittle.
In terms of priority this limited, visual only bug probably comes below lots of other items, but if there's nothing on your stack we can probably get it in for 2.9.0
3afd87a
to
57b4567
Compare
Let me know if test coverage needs to improve in any way to make this more easy to merge. I just scooped this up as some low-hanging fruit to address while triaging new issues. If we want to leave it on the vine, I'll make sure to follow up and rebase every now and again so it doesn't go stale. |
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.
Tests are almost there. Possibly comment only for the resent code change.
The QA duo can give the final judgement on whether there's enough automated tests, but to me they look enough. That would mean moving to 2.9.0 is on
cypress/e2e/po/components/ResourceList/resource-list-masthead.po.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
This also allows for renaming `createProject()` to `create()`. Doing this rename allows for the data test ids to more closely match the associated methods. Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
57b4567
to
95f4d2e
Compare
@yonasberhe23 @izaac I'm adding you as reviewers to see if there's appropriate test coverage for this change. Can one of you take a look? |
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.
the e2e tests here cover the test scenario sufficiently imo
I agree it covers the UI error spam when pushing the button multiple times. Is this going into both master and 2.9 next1 ? |
The intent is to get this merged into |
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.
@richard-cox would you like to take another look when you get the chance? |
As @rak-phillip mentioned in the summary, I believe this needs a manual check for the other use case mentioned in this comment #10748 (comment) which is not covered in our e2e tests. moving to To Test cc: @izaac |
Summary
This resets errors on the Project form if a project fails to create. The original implementation would simply push any errors returned from the server to the collection, causing errors to flood the page if someone attempted to click the "Create" button multiple times without addressing any problems.
Fixes #10748
Areas or cases that should be tested
See reproduction steps in issue #10748. Also take note of the comment for an additional case1
Checklist
Footnotes
https://github.com/rancher/dashboard/issues/10748#issuecomment-2035411554 ↩