This is a regression due to bug 289043. In editproducts.cgi at line 511: $mandatory_groups = $dbh->selectall_arrayref( 'SELECT groups.name, COUNT(bugs.bug_id) AS count FROM bugs LEFT JOIN bug_group_map ON bug_group_map.bug_id = bugs.bug_id INNER JOIN groups ON bug_group_map.group_id = groups.id WHERE groups.id IN (' . join(', ', @now_mandatory) . ') AND bugs.product_id = ? AND bug_group_map.bug_id IS NULL ' . $dbh->sql_group_by('groups.name'), {'Slice' => {}}, $product->id); This request returns no result because 'INNER JOIN groups' is called after 'LEFT JOIN bug_group_map'.

Used Inner Join in query instead of Outer to get it to return records.

Could somebody (LpSolit?) explain what the user-facing behavior is supposed to be and what it is instead? I can hardly understand this code. :-)

With the corrected SQL, a confirmation prompt will now come up if a MemberControl for any group is changed to Mandatory. The confirmation will also come up if a MemberControl is set to NA, but the bug was that changing a MemberControl to Mandatory (and no MemberControls were set to NA) then the confirmation would be bypassed.

(In reply to comment #2) > Could somebody (LpSolit?) explain what the user-facing behavior is supposed to > be and what it is instead? I can hardly understand this code. :-) No worry, I will review it. :)

Comment on attachment 311972 [details] [diff] [review] v1 >- LEFT JOIN bug_group_map >+ INNER JOIN bug_group_map >- AND bug_group_map.bug_id IS NULL ' . >+ AND bugs.product_id = ? ' . This doesn't work. We want bugs with no entry in the bug_group_map table. What you do is exactly the opposite.

There's probably a more elegant way to do this, but this will get the bugs with no entry in the bug_group_map table for the mandatory groups.

Comment on attachment 312201 [details] [diff] [review] v2 I cannot believe something as simple as this must require 5 consecutive SELECT. Must be doable with a NOT EXISTS or an UNION. I don't have the right idea yet, but this may come when I have time to investigate this a bit more. Why not look for all bugs in the given product_id and which are not in the bug_group_map table AND do an UNION with bugs which are in the bug_group_map table but not with the correct group(s)? This should make the query much simple.

Comment on attachment 312201 [details] [diff] [review] v2 If this works, doing it with subselects is fine by me. Do capitalize SELECT, though. LpSolit is right that you can do what you want with EXISTS instead of the > 0, which would probably be better. I'd like the formatting to be a bit different, also--it's very difficult to determine where that first subselect ends.

I used EXISTS instead of > 0 to clean up the query, otherwise I couldn't find a simpler way of doing the query.

Comment on attachment 313035 [details] [diff] [review] v3 Yes, this code looks great, thanks for the formatting fix, that makes it way easier to read. I'm fine with the code, LpSolit--could you tell me if this does what it's supposed to do? :-) >+ (SELECT COUNT(bugs.bug_id) >+ FROM bugs >+ WHERE bugs.product_id = ? >+ AND bugs.bug_id NOT IN >+ (SELECT bug_group_map.bug_id FROM bug_group_map >+ WHERE bug_group_map.group_id = groups.id)) One minor nit is that that second SELECT there should probably be indented to note that it's part of the NOT IN, not part of the main query there. >Index: index.cgi And I don't think this change needs to be checked in, at least not as part of this patch, yes?

(In reply to comment #10) > (From update of attachment 313035 [details] [diff] [review]) > Yes, this code looks great, thanks for the formatting fix, that makes it way > easier to read. > I'm fine with the code, LpSolit--could you tell me if this does what it's > supposed to do? :-) > >+ (SELECT COUNT(bugs.bug_id) > >+ FROM bugs > >+ WHERE bugs.product_id = ? > >+ AND bugs.bug_id NOT IN > >+ (SELECT bug_group_map.bug_id FROM bug_group_map > >+ WHERE bug_group_map.group_id = groups.id)) > One minor nit is that that second SELECT there should probably be indented to > note that it's part of the NOT IN, not part of the main query there. > >Index: index.cgi > And I don't think this change needs to be checked in, at least not as part of > this patch, yes? Sorry, the index.cgi change should not have been in there.

Comment on attachment 313035 [details] [diff] [review] v3 > if (@now_mandatory) { > $mandatory_groups = $dbh->selectall_arrayref( >+ 'SELECT groups.name, >+ (SELECT COUNT(bugs.bug_id) >+ FROM bugs >+ WHERE bugs.product_id = ? >+ AND bugs.bug_id NOT IN >+ (SELECT bug_group_map.bug_id FROM bug_group_map >+ WHERE bug_group_map.group_id = groups.id)) >+ AS count >+ FROM groups >+ WHERE groups.id IN (' . join(', ', @now_mandatory) . ') >+ AND EXISTS (SELECT bugs.bug_id FROM bugs >+ WHERE bugs.product_id = ? >+ AND bugs.bug_id NOT IN >+ (SELECT bug_group_map.bug_id FROM bug_group_map >+ WHERE bug_group_map.group_id = groups.id)) >+ ORDER BY groups.name', It's irritating that PostgreSQL requires to rewrite the whole sub-select instead of having WHERE groups.id IN (...) AND count > 0 For clarity, maybe could we drop the AND EXISTS () block completely and remove zero counts ourselves with: @$mandatory_groups = grep { $_->{count} } @$mandatory_groups; Max, what do you prefer?

Holding approval till Max's answer.

Hm, yeah, actually, for simplicity's sake I think your idea about the grep is a good one.

Elliotte, do you want to update the patch yourself, or do you prefer us to do it?

(In reply to comment #15) > Elliotte, do you want to update the patch yourself, or do you prefer us to do > it? I'll do it. I was tied up with a conference for a few days and couldn't get to it.

Simplifies SQL query by adding grep to remove groups with a count of zero.

Comment on attachment 315194 [details] [diff] [review] v4 Works fine on both trunk and 3.0. r=LpSolit

tip: Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.142; previous revision: 1.141 done 3.0.4: Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.132.2.4; previous revision: 1.132.2.3 done