389396 - Do not list series you cannot plot

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Frédéric Buclin]

Attachment: patch, v1

On the "create chart" page (chart.cgi), if you select a series which is not public and whose you are not the creator and then click the "add to list" button, nothing happens. No error is thrown, the UI is unchanged, you have no idea what happened. On b.m.o, I tried to add the same series 3 or 4 times thinking I did something wrong the previous times. The reason was simply because the series was not public. We should inform the user about that.

Note that I don't understand why series which you are not allowed to select are displayed in the <select> field anyway. As we said on IRC:

<mkanat> Ohh, waaait...so what are we saying? That the chart system sucks? No way... :-D
<LpSolit> mkanat: yeah right, must be my fault

Comment 1

[Gervase Markham [:gerv]]

You shouldn't be able to see series which are not public and of which you are not the creator. So the correct fix is not "inform the user", it's "don't display those series" (and I was under the impression that we didn't).

Gerv

Comment 2

[Max Kanat-Alexander]

Comment on attachment 273581 [details] [diff] [review]
patch, v1

Okay, yeah, we should not be showing series that you can't plot. You shouldn't even know they exist, like products you can't see.

Comment 3

[Frédéric Buclin]

gerv, these two SQL queries seem to handle group memberships differently:

Bugzilla::Chart::getVisibleSeries():

    my $serieses = $dbh->selectall_arrayref("SELECT cc1.name, cc2.name, " .
                        "series.name, series.series_id " .
                        "FROM series " .
                        "INNER JOIN series_categories AS cc1 " .
                        "    ON series.category = cc1.id " .
                        "INNER JOIN series_categories AS cc2 " .
                        "    ON series.subcategory = cc2.id " .
                        "LEFT JOIN category_group_map AS cgm " .
                        "    ON series.category = cgm.category_id " .
                        "    AND cgm.group_id NOT IN($grouplist) " .
                        "WHERE creator = " . Bugzilla->user->id . " OR " .
                        "      cgm.category_id IS NULL " . 
                   $dbh->sql_group_by('series.series_id', 'cc1.name, cc2.name, ' .
                                      'series.name'));

In this one, all series which are not restricted to groups you don't belong to are visible to you.


Bugzilla::Series->initFromDatabase():

    my @series = $dbh->selectrow_array("SELECT series.series_id, cc1.name, " .
        "cc2.name, series.name, series.creator, series.frequency, " .
        "series.query, series.is_public " .
        "FROM series " .
        "LEFT JOIN series_categories AS cc1 " .
        "    ON series.category = cc1.id " .
        "LEFT JOIN series_categories AS cc2 " .
        "    ON series.subcategory = cc2.id " .
        "LEFT JOIN category_group_map AS cgm " .
        "    ON series.category = cgm.category_id " .
        "LEFT JOIN user_group_map AS ugm " .
        "    ON cgm.group_id = ugm.group_id " .
        "    AND ugm.user_id = " . Bugzilla->user->id .
        "    AND isbless = 0 " .
        "WHERE series.series_id = $series_id AND " .
        "(is_public = 1 OR creator = " . Bugzilla->user->id . " OR " .
        "(ugm.group_id IS NOT NULL)) " . 
        $dbh->sql_group_by('series.series_id', 'cc1.name, cc2.name, ' .
                           'series.name, series.creator, series.frequency, ' .
                           'series.query, series.is_public'));

Here (and unless I misunderstand something), you have to belong to the corresponding group to be able to view the series, unless you are the reporter or the series is public. So I see several problems here:

1) why is is_public ignored in getVisibleSeries()?
2) why is group inheritance ignored in initFromDatabase()?
3) how are group restrictions in the not yet implemented category_group_map table supposed to work with is_public? Is there an OR or an AND relation between both in order to view series? initFromDatabase() seems to indicate there is an OR relationship between is_public and group restrictions, which seems weird to me.

Please clarify all these questions.

Comment 4

[Gervase Markham [:gerv]]

LpSolit: to be honest, I'm not sure. I remember saying when Joel implemented the groups system that it was all far too complicated and no-one would be able to understand it; the number of support queries since has borne this out. Perhaps this is yet another symptom of that lack of understanding, this time on my part.

It's so long since I worked with this code that I can't even remember how I wanted it to work. <sigh> I'm sorry not to be more helpful. Please make it work in any consistent way you find sensible.

Gerv

Comment 5

[Sam Fu]

Here's a solution for the getVisibleSeries method to return series that a user is eligible to see because he's 1) either the creator or 2) a member of the same group as the creator of the series.

my $serieses = $dbh->selectall_arrayref("SELECT cc1.name, cc2.name, " .
                        "series.name, series.series_id, series.frequency " .
                        "FROM series " .
                        "INNER JOIN series_categories AS cc1 " .
                        "    ON series.category = cc1.id " .
                        "INNER JOIN series_categories AS cc2 " .
                        "    ON series.subcategory = cc2.id " .
                        "LEFT JOIN user_group_map AS ugm " .
                        "    ON user_id = creator " .
                        "LEFT JOIN user_group_map AS ugm2 " .
                        "	 ON ugm.group_id = ugm2.group_id " .
                        "WHERE ugm2.user_id = " . Bugzilla->user->id . " OR " .
                        "      creator IS NULL " . 
                   $dbh->sql_group_by('series.series_id', 'cc1.name, cc2.name, ' .
                                      'series.name'));

Comment 6

[Sam Fu]

Actually, to make this more refined we need to explicitly remove selected data from the editbugs group b/c by default everyone is part of that group.  Also there was an extraneous user_group_map in the query I submitted above.

my $serieses = $dbh->selectall_arrayref("SELECT cc1.name, cc2.name, " .
                        "series.name, series.series_id, series.frequency " .
                        "FROM series " .
                        "INNER JOIN series_categories AS cc1 " .
                        "    ON series.category = cc1.id " .
                        "INNER JOIN series_categories AS cc2 " .
                        "    ON series.subcategory = cc2.id " .
                        "LEFT JOIN user_group_map AS ugm " .
                        "    ON user_id = creator " .
                        "LEFT JOIN groups ON groups.id = ugm.group_id " .
                        "WHERE (ugm.user_id = " . Bugzilla->user->id . " OR " .
                        "      creator IS NULL) AND (groups.name != 'editbugs' OR ugm.group_id IS NULL) " . 
                   $dbh->sql_group_by('series.series_id', 'cc1.name, cc2.name, ' .
                                      'series.name'));

Comment 7

[Sam Fu]

Hey folks, I'm sorry to harp on this topic, but I forgot to include the group inheritance in the calls above.  Here's another modification that includes inheritance:

my $serieses = $dbh->selectall_arrayref("SELECT cc1.name, cc2.name, " .
                        "series.name, series.series_id, series.frequency " .
                        "FROM series " .
                        "INNER JOIN series_categories AS cc1 " .
                        "    ON series.category = cc1.id " .
                        "INNER JOIN series_categories AS cc2 " .
                        "    ON series.subcategory = cc2.id " .
                        "LEFT JOIN user_group_map AS ugm " .
                        "    ON user_id = creator " .
                        "LEFT JOIN groups ON groups.id = ugm.group_id " .
                        "LEFT JOIN group_group_map AS ggm ON ugm.group_id = ggm.grantor_id " .
                        "LEFT JOIN user_group_map AS ugm2 ON ggm.member_id = ugm2.group_id " .
                        "WHERE ((creator = " . Bugzilla->user->id . " OR " . "ugm2.user_id = " . Bugzilla->user->id .
                        "      creator IS NULL) AND (groups.name != 'editbugs')) OR series.is_public = 1 " . 
                   $dbh->sql_group_by('series.series_id', 'cc1.name, cc2.name, ' .
                                      'series.name'));

Comment 8

[Frédéric Buclin]

Sam, could you attach your changes as a patch?

Comment 9

[Frédéric Buclin]

(In reply to comment #6)
> Actually, to make this more refined we need to explicitly remove selected data
> from the editbugs group b/c by default everyone is part of that group.

No, not everybody is necessarily part of this group (despite it's the default), and there is no reason to consider this group as special.

Comment 10

[GavinS]

I hit this recently - how should the admin user be treated in this case?

I am admin, and I can see series that another admin created in the list, but I get the failure when trying to select it.

(From a bz-requirements pov:)

 - should an admin always be able to see all series? 

 - should an admin always be able to use and chart all series?

Comment 11

[Frédéric Buclin]

By reading the code in chart.cgi, Chart.pm and Series.pm, I managed to answer my own questions.

(In reply to comment #3)
> 1) why is is_public ignored in getVisibleSeries()?

That's a bug. is_public = 0 means that the creator wants to be the single one to be able to view his charts (like a saved search you don't want to share).


> 2) why is group inheritance ignored in initFromDatabase()?

That's a bug, getVisibleSeries() is much clever.


> 3) how are group restrictions in the not yet implemented category_group_map
> table supposed to work with is_public? Is there an OR or an AND relation
> between both in order to view series?

It's an AND relation. is_public means "share with other members", as explained above. If is_public = 0, the chart is yours and only viewable by you. Period. If is_public = 1, you agree to let other members view your chart, and if the category your chart belongs to is not restricted to a group (which is currently the case as the UI to restrict charts to groups doesn't exist yet), everybody else can view it, else only members of the group the category is restricted to. This is similar to "share my saved search with group X". Only members of group X can run your shared search, not other members.

Things become much clearer and more logical, now. :) Patch coming soon.

Comment 12

[Frédéric Buclin]

Attachment: patch, v2

Now only charts you can view are listed. Seems to work correctly on my installation.

Comment 13

[Frédéric Buclin]

Comment on attachment 389200 [details] [diff] [review]
patch, v2

If you don't have time to review this patch, please tell me.

Comment 14

[David Lawrence [:dkl]]

(In reply to comment #13)
> (From update of attachment 389200 [details] [diff] [review])
> If you don't have time to review this patch, please tell me.

I have just returned from a week off so I should be able to look at this on Monday EDT.

Dave

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 389200 [details] [diff] [review]
patch, v2

I have reviewed this patch and it works and looks correct. One question is that I cannot see anywhere in the code base what any values are ever added to the category_group_map table. Can you point me at where the code is adding/removing values from the table or is the table always just empty and only the series.is_public is ever really used?

Dave

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
> I cannot see anywhere in the code base what any values are ever added to the
> category_group_map table.

That's because bug 276230 isn't implemented yet (which is blocked by this bug). The DB table was added a long time ago, despite there is no UI yet. So yes, this is confusing.

Comment 17

[Frédéric Buclin]

tip:

Checking in Bugzilla/Chart.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v  <--  Chart.pm
new revision: 1.18; previous revision: 1.17
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.19; previous revision: 1.18
done


3.4.1:

Checking in Bugzilla/Chart.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v  <--  Chart.pm
new revision: 1.17.2.1; previous revision: 1.17
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.18.2.1; previous revision: 1.18
done

Comment 18

[Max Kanat-Alexander]

Added to the 3.4.2 release notes in bug 515509.