179494 - requesting super-review didn't work well

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

<biesi> myk: could you take a look at bug 102848, looks like bugzilla did
soemthing wrong there...
 <biesi> myk: the patch had review+ from ducarroz, I wanted to request
superreview from bienvenu...
 <biesi> myk: also, the process_bug page told me that no mail was sent when I
added the superreview? status, shouldn't at least the requestee have got mail?
 <biesi> myk: in addition, bugzilla mailed me: "Christian Biesinger
<[e-mail]> has approved your request for review" wtf?

additionally, the requests.cgi page now shows two requests for the same status
on the same attachment.

Comment 1

[timeless]

*** Bug 179495 has been marked as a duplicate of this bug. ***

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

ah yeah, maybe I should mention the actual issue here...
the attachment flag box looks like this:
cbiesinger: superreview? (bienvenu)
cbiesinger: superreview? (bienvenu)
cbiesinger: review+ 

I would have expected only one superreview? and the review+ should be ducarroz's
not mine.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes problem

This patch fixes the problem, but it may not fix it all the way, and I think it
needs to be carefully scrutinized.

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 105884 [details] [diff] [review]
patch v1: fixes problem

The logic here looks right. Skip if:

- the status is the same, and
  - there couldn't have been a user; or
  - there was originally   a user, and tha tuser is still the same

r=bbaetz

Comment 5

[Dave Miller [:justdave]]

a=justdave

Comment 6

[[e-mail]]

>a=justdave

myk goes to all the trouble to upgrade Bugzilla, and for what?

Comment 7

[Jeff Hedlund]

this also fixes an issue where if you edit an attachment with a set flag and
submit without changing anything, it sends an additional email about the flag
being set.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

that fixes it? great... earlier today, I had experienced that bug on bmo

Comment 9

[Bradley Baetz (:bbaetz)]

..except that myk told me that this patch was already on bmo...

Comment 10

[Bradley Baetz (:bbaetz)]

Actually, theres another problem - bug 179643 is a separate issue. Going to
comment there now

Comment 11

[Bradley Baetz (:bbaetz)]

No, forget that - it is the same issue

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes problem some more

This patch deals with additional cases of this bug that have been reported
since the first patch was applied to b.m.o Monday morning.  This patch has also
been applied to b.m.o.

The patch fixes the problem by trimming the email address field, which is given
a space on the end by the user matching code in User::match_field.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

cc:ing Erik for explanation of User::match_field's behavior.  Erik, why does
User::match_field add a space to an email address for a single-user field?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

*** Bug 179643 has been marked as a duplicate of this bug. ***

Comment 15

[Erik Stambaugh]

Attachment: Fixes the appended-space problem

re: comment 13

> Erik, why does User::match_field add a space to an email address for a
> single-user field?

It was a brain-dead delimiter being tacked on to everything, which worked in
everything but RT.  This patch should fix it.

Comment 16

[Bradley Baetz (:bbaetz)]

Comment on attachment 106392 [details] [diff] [review]
Fixes the appended-space problem

Rather than testing the length, just use the string directly - its quicker,
since you just want to know if the string is empty or not

Comment 17

[Erik Stambaugh]

Attachment: Fixes appended-space problem (new)

re: comment 16

> Rather than testing the length, just use the string directly - its quicker,
> since you just want to know if the string is empty or not

I guess I get into the habit of testing the length because the string could
possibly contain something like '0', especially if logins stop being email
addresses.  Boolean test against the string doesn't just fail if it's empty.

Anyway, this patch does the boolean string test instead.

Comment 18

[Bradley Baetz (:bbaetz)]

Comment on attachment 106437 [details] [diff] [review]
Fixes appended-space problem (new)

This looks fine, r=bbaetz. Can myk check that this solves teh other problem w/o
needing this workarround? It should, but....

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: superset of fixes

This patch combines not_erik's and my approaches, since each fixes only part of
the problem.  not_erik's fix deals with the general case, while my fix targets
specific kinds of transactions for which the earlier logic (for determining
whether or not the flags had changed) was faulty.

Comment 20

[Bradley Baetz (:bbaetz)]

Comment on attachment 106559 [details] [diff] [review]
patch v5: superset of fixes

r=bbaetz, but change the &::trim to trim, and |use Bugzilla::Util;| if you need
to include that to get this working

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.6; previous revision: 1.5
done

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: corrections per bbaetz' last comment

Somehow I missed bbaetz' last comment when I checked this in.  This patch fixes
that.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.7; previous revision: 1.6
done