Fix bugs from wrong event targets being used in event handlers.

Review Request #13133 — Created July 3, 2023 and submitted

Information

Review Board
release-5.0.x

Reviewers

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 undefined when 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 from model.get('attachmentHistoryID') was correct.
  • Tested the expand and collapse buttons for the diffviewer and review
    request page.
Summary ID
Fix bugs from wrong event targets being used in event handlers.
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 …

daviddavid
david
  1. 
      
  2. What exactly is the case where the ID in the element is undefined? Is it when the attachment being uploaded was just newly created?

    1. Not quite, the case would happen almost randomly and I wouldn't always be able to reproduce it. I would have a review request with multiple file attachments, and when clicking on the "update" action for any of the file attachments, it would sometimes be undefined and sometimes not. I would just cycle between clicking different attachments' "update" action until I got one that was undefined. The bug can be reproduced regardless of whether the review request is in draft mode or not, or whether the file attachments are drafts or published ones. Was also able to reproduce when there was only one file attachment in the review request, but I noticed it didn't happen as often as when there were multiple file attachments.

    2. Hmm, that makes me uncomfortable. Mind spending some time seeing if you can figure out why that's failing?

    3. Sure, I'll look into it more.

  3. 
      
maubin
david
  1. 
      
  2. Show all issues

    Can we switch these two lines (put the newline between the event bubbling control and the stuff related to the dialog)?

  3. 
      
maubin
david
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.0.x (70a6263)