flake8
passed.
JSHint
passed.
Fix bugs from wrong event targets being used in event handlers.
Review Request #13133 — Created July 3, 2023 and submitted
We ran into a bug where sometimes updating a file attachment would create a
brand new one instead of replacing the original one. This happens because we
rely on an HTML attribute to set the file attachment history ID used for
uploading the attachment, which would sometimes beundefined. The
attribute would beundefinedwhen clicking directly on the file upload icon,
which would cause the event target of the click to be the icon element instead
of the<a>element which holds the attachment history ID attribute. To fix
this, we grab the attachment history ID from the model instead. An alternative
solution would be to use$(e.currentTarget)instead of$(e.target)to grab
the event target, which will always refer to the element that the event
handler is attached to instead of the actual element that was clicked.I audited the rest of the codebase for any similar problems and updated
some areas where we caught the problem but used more expensive solutions.
- Ran JS unit tests.
- Tested updating file attachments, uploading new ones, and deleting.
- Hit the case where updating a file attachment would create a new one, saw
that the value from$(e.target).data('attachment-history-id')was
undefined but the value frommodel.get('attachmentHistoryID')was correct. - Tested the expand and collapse buttons for the diffviewer and review
request page.
| Summary | ID |
|---|---|
| 3faf46dc8847c7cd816eb49de620b8ff4e2b73cc |
| Description | From | Last Updated |
|---|---|---|
|
Can we switch these two lines (put the newline between the event bubbling control and the stuff related to the … |
 david
|
- Summary:
-
Fix bug where updating a file attachment creates a new one.Fix bugs from wrong event targets being used in event handlers.
- Description:
-
~ Sometimes updating a file attachment would create a brand new one instead of
~ replacing the original one. This happened because we would rely on an HTML ~ attribute to set the file attachment history ID used for uploading the ~ attachment, which would sometimes be undefined. To fix this we get the~ attachment history ID from the file attachment model instead. ~ We ran into a bug where sometimes updating a file attachment would create a
~ brand new one instead of replacing the original one. This happens because we ~ rely on an HTML attribute to set the file attachment history ID used for ~ uploading the attachment, which would sometimes be undefined. The~ attribute would be undefinedwhen clicking directly on the file upload icon,+ which would cause the event target of the click to be the icon element instead + of the <a>element which holds the attachment history ID attribute. To fix+ this, we grab the attachment history ID from the model instead. An alternative + solution would be to use $(e.currentTarget)instead of$(e.target)to grab+ the event target, which will always refer to the element that the event + handler is attached to instead of the actual element that was clicked. + + I audited the rest of the codebase for any similar problems and updated
+ some areas where we caught the problem but used more expensive solutions. - Testing Done:
-
- Ran JS unit tests.
- Tested updating file attachments, uploading new ones, and deleting.
- Hit the case where updating a file attachment would create a new one, saw
that the value from$(e.target).data('attachment-history-id')was
undefined but the value frommodel.get('attachmentHistoryID')was correct.
+ - Tested the expand and collapse buttons for the diffviewer and review
request page.
- Commits:
-
Summary ID 68cc5679cb9e246d07f6073b4c3c95d54ad564df 880cb43d2d717813458f426f82c7cc10c6eca8d6
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.