1215501 - [modal ui only?] When moving a bug to a different product and removing a security-group, the other product's security group gets added automatically anyway

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Bug Creation/Editing, defect)

Description

[:Gijs (he/him)]

This is essentially bug 1182500 regressing, AFAICT.

(this just happened to me with bug 1215456)

Comment 1

[David Lawrence [:dkl]]

Taking a look.

Comment 2

[David Lawrence [:dkl]]

I am not able to reproduce this locally. If I uncheck the default core-security group for Core, it is not added back automatically for me. And firefox-core-security is removed as expected since it is not valid for Core product.

I may need to try this on production to be sure.

dkl

Comment 3

[:Gijs (he/him)]

(In reply to David Lawrence [:dkl] from comment #2)
> I am not able to reproduce this locally. If I uncheck the default
> core-security group for Core, it is not added back automatically for me. And
> firefox-core-security is removed as expected since it is not valid for Core
> product.
> 
> I may need to try this on production to be sure.
> 
> dkl

No, the problem is that when the product is still in firefox, you remove the security group, then you move it to a different product, and at that point the security group gets reinstated (to core-security, which I can't remove because I'm not in the right security group on bmo).

Comment 4

[David Lawrence [:dkl]]

(In reply to :Gijs Kruitbosch from comment #3)
> No, the problem is that when the product is still in firefox, you remove the
> security group, then you move it to a different product, and at that point
> the security group gets reinstated (to core-security, which I can't remove
> because I'm not in the right security group on bmo)

Ok. I see more now of what you are saying. I also tried it that way with a test user that is in the firefox security group but not core-security and I was still unable to make it fail in the way you describe.

(as a privileged user that is not an admin)
1. Enter or go to a Firefox bug marked private to firefox-core-security.
2. Click 'edit' and uncheck the firefox-core-security checkbox.
3. On the same page, choose Core as a new product with component Security.
4. The core-security group is now checked by default, but editable.
5. Uncheck the core-security checkbox before submitting.
6. Click submit and observe that the bug is now fully public under the new product.
   The core-security group is not on and is not present as the test user is not in the
   core-security group.

Am I missing a step somewhere? glob, any ideas on this?

dkl

Comment 5

[:Gijs (he/him)]

(In reply to David Lawrence [:dkl] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > No, the problem is that when the product is still in firefox, you remove the
> > security group, then you move it to a different product, and at that point
> > the security group gets reinstated (to core-security, which I can't remove
> > because I'm not in the right security group on bmo)
> 
> Ok. I see more now of what you are saying. I also tried it that way with a
> test user that is in the firefox security group but not core-security and I
> was still unable to make it fail in the way you describe.
> 
> (as a privileged user that is not an admin)
> 1. Enter or go to a Firefox bug marked private to firefox-core-security.
> 2. Click 'edit' and uncheck the firefox-core-security checkbox.
> 3. On the same page, choose Core as a new product with component Security.
> 4. The core-security group is now checked by default, but editable.

Yes, but I didn't expect this bit. Why is it now checked? It shouldn't be. I didn't know that moving the bug to a different product was going to re-mark the bug as sec-sensitive, so I submitted, and then the bug got "stuck" in core-security until someone in that group removed it again.

Comment 6

[:glob ✱]

(In reply to :Gijs Kruitbosch from comment #5)
> Yes, but I didn't expect this bit. Why is it now checked? It shouldn't be.

it's checked because the code that builds the group checkboxes looks at the server-side state of the bug, and doesn't take into account the client-side change making the bug public before a new product is selected.  this is a bug.

Comment 7

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Yes, but I didn't expect this bit. Why is it now checked? It shouldn't be.
> 
> it's checked because the code that builds the group checkboxes looks at the
> server-side state of the bug, and doesn't take into account the client-side
> change making the bug public before a new product is selected.  this is a
> bug.

Ah thanks glob. Now that I understand the issue it makes more sense. I can still take a look at solving this now that I know where to look.

dkl

Comment 8

[:Gijs (he/him)]

Sorry for the ping, but is there any progress here, dkl? This just bit me again in bug 1230080.

Comment 9

[David Lawrence [:dkl]]

(In reply to :Gijs Kruitbosch (PTO Nov. 17-20 inclusive) from comment #8)
> Sorry for the ping, but is there any progress here, dkl? This just bit me
> again in bug 1230080.

Sorry this fell through the cracks all this time. Working on it now.

dkl

Comment 10

[David Lawrence [:dkl]]

Attachment: 1215501_1.patch

Comment 11

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

Comment on attachment 8812926 [details] [diff] [review]
1215501_1.patch

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

r=dylan

testing this was a lot of setup. wish we had test cases for all these variations of admin/non-admin and various groups. :-/

Comment 12

[David Lawrence [:dkl]]

(In reply to Dylan Hardison [:dylan] from comment #11)
> Comment on attachment 8812926 [details] [diff] [review]
> 1215501_1.patch
> 
> Review of attachment 8812926 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=dylan
> 
> testing this was a lot of setup. wish we had test cases for all these
> variations of admin/non-admin and various groups. :-/

Agreed :-/ Should be a quarter goal IMO in the near future to get a good set of tests for the modal UI.

To https://github.com/mozilla-bteam/bmo.git
   6d01972..2dec6dc  master -> master

dkl