Closed Bug 250967 Opened 19 years ago Closed 19 years ago

Requested requesteeless flags get updated spuriously

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

2.17.7
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: jouni, Assigned: jouni)

References

Details

Attachments

(1 file)

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.
Requesting branch blocking; I think this is severe enough. Besides, I think the
patch is going to be pretty straightforward...
Status: NEW → ASSIGNED
Flags: blocking2.18?
Target Milestone: --- → Bugzilla 2.18
A-ha. This only occurs with requested flags that have no requestee.
Summary: Flags in ? state get updated spuriously → Requested requesteeless flags get updated spuriously
Attached patch v1Splinter Review
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.
Attachment #152911 - Flags: review?(justdave)
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 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.
Attachment #152911 - Flags: review+
> Ack. Requesteeble?!

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

Gerv
So you mean we should request Mike add something like

  Bugzilla: pushing the English language to exciting new frontiers.

to the new website <wink>?
Flags: approval?
Flags: approval2.18?
*** Bug 224019 has been marked as a duplicate of this bug. ***
Flags: blocking2.18? → blocking2.18+
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.
Attachment #152911 - Flags: review?(justdave) → review?
(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 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.
Attachment #152911 - Flags: review?
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
checked in on both
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.