1255272 - Adding a flag via the MozReview batch-attachment API doesn't CC the user

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: MozReview Integration, defect)

Description

[Mark Côté [:mcote]]

I created an attachment thusly (API key is local, no secret here):

{'bug_id': 1, 'api_key': u'KA6yClY190w5aBk1eSAFQ2lV2sos22XZ2xMb3kGr', 'attachments': [{'comment': u'Review commit: http://172.16.130.136:59999/r/2/diff/#index_header\nSee other reviews: http://172.16.130.136:59999/r/2/', 'file_name': 'reviewboard-2-url.txt', 'summary': u'MozReview Request: Bug 1 - Update HACKING.md. r?jrandom', 'flags': [{'status': '?', 'requestee': u'[e-mail]', 'name': 'review', 'new': True}], 'content_type': 'text/x-review-board-request', 'data': u'http://172.16.130.136:59999/r/2/diff/#index_header'}, {'comment': u'Review commit: http://172.16.130.136:59999/r/3/diff/#index_header\nSee other reviews: http://172.16.130.136:59999/r/3/', 'file_name': 'reviewboard-3-url.txt', 'summary': u'MozReview Request: Bug 1 - Fixed that thing. r?jrandom r?mcote', 'flags': [{'status': '?', 'requestee': u'[e-mail]', 'name': 'review', 'new': True}, {'status': '?', 'requestee': u'[e-mail]', 'name': 'review', 'new': True}], 'content_type': 'text/x-review-board-request', 'data': u'http://172.16.130.136:59999/r/3/diff/#index_header'}, {'comment': u'The README.md has been updated to be more README like.  People reading the\nREADME will know they are reading a README.\n\nReview commit: http://172.16.130.136:59999/r/4/diff/#index_header\nSee other reviews: http://172.16.130.136:59999/r/4/', 'file_name': 'reviewboard-4-url.txt', 'summary': u'MozReview Request: Bug 1 - Update README.md. r?jrandom', 'flags': [{'status': '?', 'requestee': u'[e-mail]', 'name': 'review', 'new': True}], 'content_type': 'text/x-review-board-request', 'data': u'http://172.16.130.136:59999/r/4/diff/#index_header'}]}

which returned

{'attachments_modified': {}, 'attachments_created': {'1': {'description': 'MozReview Request: Bug 1 - Update HACKING.md. r?jrandom', 'creator': '[e-mail]', 'is_obsolete': 0, 'is_patch': 0, 'creation_time': <DateTime '20160310T03:53:39' at 7f265b6aedd0>, 'last_change_time': <DateTime '20160310T03:53:39' at 7f265b6aee18>, 'bug_id': 1, 'flags': [{'status': '?', 'requestee': '[e-mail]', 'name': 'review', 'type_id': 2, 'id': 1, 'setter': '[e-mail]'}], 'content_type': 'text/x-review-board-request', 'file_name': 'reviewboard-2-url.txt', 'attacher': '[e-mail]', 'summary': 'MozReview Request: Bug 1 - Update HACKING.md. r?jrandom', 'data': <xmlrpclib.Binary instance at 0x7f265b6aed88>, 'id': 1, 'is_private': 0, 'size': 50}, '3': {'description': 'MozReview Request: Bug 1 - Update README.md. r?jrandom', 'creator': '[e-mail]', 'file_name': 'reviewboard-4-url.txt', 'is_patch': 0, 'creation_time': <DateTime '20160310T03:53:39' at 7f265b6b20e0>, 'summary': 'MozReview Request: Bug 1 - Update README.md. r?jrandom', 'bug_id': 1, 'flags': [{'requestee': '[e-mail]', 'status': '?', 'name': 'review', 'type_id': 2, 'id': 4, 'setter': '[e-mail]'}], 'last_change_time': <DateTime '20160310T03:53:39' at 7f265b6b2098>, 'content_type': 'text/x-review-board-request', 'is_obsolete': 0, 'attacher': '[e-mail]', 'data': <xmlrpclib.Binary instance at 0x7f265b6aef80>, 'id': 3, 'is_private': 0, 'size': 50}, '2': {'description': 'MozReview Request: Bug 1 - Fixed that thing. r?jrandom r?mcote', 'creator': '[e-mail]', 'is_obsolete': 0, 'is_patch': 0, 'creation_time': <DateTime '20160310T03:53:39' at 7f265b6aeef0>, 'last_change_time': <DateTime '20160310T03:53:39' at 7f265b6aef38>, 'bug_id': 1, 'flags': [{'requestee': '[e-mail]', 'status': '?', 'name': 'review', 'type_id': 2, 'id': 2, 'setter': '[e-mail]'}, {'requestee': '[e-mail]', 'status': '?', 'name': 'review', 'type_id': 2, 'id': 3, 'setter': '[e-mail]'}], 'content_type': 'text/x-review-board-request', 'file_name': 'reviewboard-3-url.txt', 'attacher': '[e-mail]', 'summary': 'MozReview Request: Bug 1 - Fixed that thing. r?jrandom r?mcote', 'data': <xmlrpclib.Binary instance at 0x7f265b6aeea8>, 'id': 2, 'is_private': 0, 'size': 50}}}

I then added another review flag:

{'bug_id': 1, 'api_key': u'KA6yClY190w5aBk1eSAFQ2lV2sos22XZ2xMb3kGr', 'attachments': [{'comment': u'', 'file_name': 'reviewboard-4-url.txt', 'attachment_id': 3, 'flags': [{'status': '?', 'requestee': u'[e-mail]', 'name': 'review', 'new': True}], 'summary': u'MozReview Request: Bug 1 - Update README.md. r?jrandom'}]}

This appeared to work:

{'attachments_modified': {'3': {'changes': {'flagtypes.name': {'removed': '', 'added': 'review?([e-mail])'}}, 'id': 3, 'last_change_time': <DateTime '20160310T03:55:28' at 7f265bb35320>}}, 'attachments_created': {}}

Indeed, I see that the review flag is set to both [e-mail] (the original reviewer) and [e-mail] (the new one).

However, [e-mail] is *not* CCed on the bug.  They should be, as per normal flag additions.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

This is the tale of two bug objects.

Consider that this bug occurs only in this block (when there is an attachment_id)
and not

https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/MozReview/lib/WebService.pm#L79-L145


There are clearly three calls to $attachment_obj->set_flags(), which calls Bugzilla::Flag->set_flag(),
which should eventually call $bug->add_cc. However, the $bug it calls is the bug created by Bugzilla::Attachment,
reachable as $attachment_obj->bug. $bug and $attachment_obj->bug are two independent objects,
and when we call $bug->add_cc(), it queues up the sql update or insert. No action takes place until the update() method on the object is called. But we only ever call update() on $bug.

In this branch of code, we need to swap out $bug for $attachment_obj bug, and all should be right with the world.

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1255272_1.patch

After exhausting more complicated solutions, here's a horrible hack.

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8729700 [details] [diff] [review]
1255272_1.patch

Review of attachment 8729700 [details] [diff] [review]:
-----------------------------------------------------------------

Looks correct but ran out of time today. Will review properly over weekend of first thing Monday morning.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 8729700 [details] [diff] [review]
1255272_1.patch

Review of attachment 8729700 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 5

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   3c360d8..c55400f  master -> master