Last Comment Bug 389396 - Do not list series you cannot plot
: Do not list series you cannot plot
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Reporting/Charting (show other bugs)
: 3.0
: All All
: -- minor with 1 vote (vote)
: Bugzilla 3.4
Assigned To: Frédéric Buclin
: default-qa
Mentors:
chart.cgi
Depends on:
Blocks: 276230
  Show dependency treegraph
 
Reported: 2007-07-24 06:20 PDT by Frédéric Buclin
Modified: 2010-02-28 10:58 PST (History)
7 users (show)
LpSolit: approval+
LpSolit: approval3.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (4.41 KB, patch)
2007-07-24 06:20 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Review
patch, v2 (3.35 KB, patch)
2009-07-17 14:19 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Review

Description Frédéric Buclin 2007-07-24 06:20:45 PDT
Created attachment 273581 [details] [diff] [review]
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] 2007-07-24 16:15:43 PDT
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 2007-07-25 04:14:20 PDT
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 2008-03-24 16:09:18 PDT
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] 2008-04-02 03:27:32 PDT
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 2008-04-22 14:58:01 PDT
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 2008-04-22 15:22:46 PDT
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 2008-04-22 17:22:52 PDT
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 2008-04-23 04:45:39 PDT
Sam, could you attach your changes as a patch?
Comment 9 Frédéric Buclin 2008-04-27 14:28:32 PDT
(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 2009-03-16 03:39:32 PDT
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 2009-07-17 08:49:34 PDT
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 2009-07-17 14:19:05 PDT
Created attachment 389200 [details] [diff] [review]
patch, v2

Now only charts you can view are listed. Seems to work correctly on my installation.
Comment 13 Frédéric Buclin 2009-08-15 03:11:56 PDT
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] 2009-08-15 21:28:22 PDT
(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] 2009-08-17 13:29:10 PDT
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 2009-08-17 15:44:58 PDT
(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 2009-08-17 16:02:04 PDT
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 2009-09-10 16:49:22 PDT
Added to the 3.4.2 release notes in bug 515509.

Note You need to log in before you can comment on or make changes to this bug.