When updating an alert, don't also update the question #36866
Conversation
rafpaf
added
the
backport
rafpaf
requested review from
qnkhuat,
a team and
npfitz
and removed request for
a team
|
dont think this needs to be backported? |
rafpaf
added
no-backport
@perivamsi: I previously labeled this bugfix with |
rafpaf
force-pushed
the
update-alert-not-question
branch
from
5708acf
to
9a7b787
Compare
|
|
||
| cy.log("Change the frequency of the alert to weekly"); | ||
|
|
||
| cy.get(".Modal--full").within(() => { |
There was a problem hiding this comment.
Another approach would be to add some attributes to this full-page modal, such as role="dialog" and aria-labelledby="Edit your alert" – or rather, the modal could have a name as a prop, and then we'd do aria-labelledby={name} – and then we could do cy.findByRole('dialog', {name: "Edit your alert"}), which is more user-centric. I'd be glad to do that, if reviewers prefer.
For now, I've gone with a simpler approach. I see the codebase already uses cy.get('.Modal--full') a few times, like here.
There was a problem hiding this comment.
good questions!
- cy.get() is strongly discouraged. it exists in older tests, we shouldn't copy that pattern.
- accessible roles and other attributes that play nicely with react testing library are highly favored, but there are some existing helpers and tests that rely on a single "dialog" role being present, so if you're in a situation with multiple dialogs it could get messy.
- the easiest and safest option from a testing standpoint is using
data-testid, it makes tests nice and easy to read because you can make it very specific, it's always safe to add, it's most likely to be stable, but it doesn't add any nice accessibility feature to the application.
TLDR be very bold:
There was a problem hiding this comment.
accessible roles and other attributes that play nicely with react testing library are highly favored, but there are some existing helpers and tests that rely on a single "dialog" role being present, so if you're in a situation with multiple dialogs it could get messy.
I went ahead and tidied up some of the messiness around testing modals. The tidying happens in this new refactoring PR. This "When updating an alert" PR now uses that refactoring PR as its base.
rafpaf
force-pushed
the
update-alert-not-question
branch
from
42dc8f6
to
f9f6bb1
Compare
rafpaf
force-pushed
the
update-alert-not-question
branch
from
f9f6bb1
to
c02eeef
Compare
| const { modifiedAlert } = this.state; | ||
|
|
||
| await apiUpdateQuestion(); |
There was a problem hiding this comment.
This is the core behavioral change in this PR: when we update an alert, we shouldn't also update the question.
In the past, it might have made sense to automatically update the question when the alert is updated. But I think it no longer makes sense to do this. The reason is that it is now impossible to update an alert if the corresponding question has not yet been saved.
Fixes #36395
Description
On master, when you edit an alert, an additional
PUTrequest is to send to update the card. This is not needed, because you can't edit an alert without first saving the question. On this branch, this request is not done.How to verify
On master:
On this branch, do the same thing and note that there is no additional request.
Demos
PUTrequest to /api/card:idPUTrequest to /api/card/:idChecklist
Since this PR removes unneeded functionality, I haven't written a test for this. Of course, reviewers, please let me know if you think a test would make sense here.I've now written a test, per @npfitz's request.