250967 - Requested requesteeless flags get updated spuriously

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Jouni Heikniemi]

Flags in the requested state get updated spuriously whenever someone changes the
bug/attachment the request is put on.

To reproduce:

1. Log in with account A
2. Make a request (say "approval?")
3. Reload the show_bug page; see "a: approval [ ? ]"
4. Log in with account B
5. Make any change on the bug page and commit
6. Reload the show_bug page; see "b: approval [ ? ]"

Similarly with attachment.cgi's edit screen.

I'll patch.

Comment 1

[Jouni Heikniemi]

Requesting branch blocking; I think this is severe enough. Besides, I think the
patch is going to be pretty straightforward...

Comment 2

[Jouni Heikniemi]

A-ha. This only occurs with requested flags that have no requestee.

Comment 3

[Jouni Heikniemi]

Attachment: v1

Ok, this fixed it for me. The culprit was just one huge boolean expression with
a minor mistake (wrong checking of empty $flag->{'requestee'}) thrown in. It
was pretty unreadable, so I rewrote that section. This should be a safe one.

Comment 4

[Jouni Heikniemi]

Oh, and the patch applies cleanly to both trunk and branch -- if it passes
review, can I get approval(s) while you're at it?

Comment 5

[Christian Reis]

Comment on attachment 152911 [details] [diff] [review]
v1


Looks great.

>Index: Bugzilla/Flag.pm
>-          && (!$flag->{'type'}->{'is_requesteeble'} 

Ack. Requesteeble?! Something that can be specifically requested to a
requestee? 

>+        # Requestee is considered changed, if all of the following apply:
>+        # 1. Flag status is '?' (requested)
>+        # 2. Flag can have a requestee
>+        # 3. The requestee specified on the form is different from the 
>+        #    requestee specified in the db.

Well, the outcome of your patch is that the logic is actually readable, which
is nice because it makes having comments unnecessary :-) Consider whether
you really want this comment here or not.

Comment 6

[Gervase Markham [:gerv]]

> Ack. Requesteeble?!

I believe Myk has defined the terminology here in the past...

Gerv

Comment 7

[Christian Reis]

So you mean we should request Mike add something like

  Bugzilla: pushing the English language to exciting new frontiers.

to the new website <wink>?

Comment 8

[Dave Miller [:justdave]]

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

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 152911 [details] [diff] [review]
v1

I'd like to see the second review on this (primarily because it needs to land
on the branch) but I don't have time to give it an adequate one.

Comment 10

[Jouni Heikniemi]

(In reply to comment #5)
> Ack. Requesteeble?! Something that can be specifically requested to a
> requestee? 

Exactly ;-(

> Well, the outcome of your patch is that the logic is actually readable, which
> is nice because it makes having comments unnecessary :-) Consider whether
> you really want this comment here or not.

It's rare to see review comments like this. ;-) Unless the 2nd reviewer agrees
with you, I'll leave the comment alone; I think it's a good idea to describe key
logic in terms other than code, especially as code terminology like
"is_requesteeble" isn't immediately clear (and particularly, can be confused
with is_requestable, which is also valid). 

Comment 11

[Joel Peshkin]

Comment on attachment 152911 [details] [diff] [review]
v1

r=joel, 
The logic works as Jouni states.  I don't have a good way to test this right
now, but it reads very clean.

Comment 12

[Joel Peshkin]

checked in on both