Do not list series you cannot plot

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Reporting/Charting
--
minor
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

(Blocks: 1 bug)

Bugzilla 3.4
Bug Flags:
approval +
approval3.4 +

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

3.35 KB, patch
dkl
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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
Attachment #273581 - Flags: review?(mkanat)
Attachment #273581 - Flags: review?(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

10 years ago
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.
Attachment #273581 - Flags: review?(mkanat)
Attachment #273581 - Flags: review?(gerv)
Attachment #273581 - Flags: review-
(Assignee)

Comment 3

9 years ago
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.
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

9 years ago
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

9 years ago
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

9 years ago
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'));
(Assignee)

Comment 8

9 years ago
Sam, could you attach your changes as a patch?
(Assignee)

Comment 9

9 years ago
(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.
(Assignee)

Updated

8 years ago
Target Milestone: Bugzilla 3.2 → ---

Comment 10

8 years ago
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?
(Assignee)

Comment 11

8 years ago
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.
Target Milestone: --- → Bugzilla 3.4
(Assignee)

Comment 12

8 years ago
Created attachment 389200 [details] [diff] [review]
patch, v2

Now only charts you can view are listed. Seems to work correctly on my installation.
Attachment #273581 - Attachment is obsolete: true
Attachment #389200 - Flags: review?(gerv)
(Assignee)

Updated

8 years ago
Summary: No error is thrown when selecting a series you cannot plot → Do not list series you cannot plot
(Assignee)

Updated

8 years ago
Blocks: 276230
(Assignee)

Comment 13

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

If you don't have time to review this patch, please tell me.
Attachment #389200 - Flags: review?(justdave)
Attachment #389200 - Flags: review?(gerv)
Attachment #389200 - Flags: review?(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 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
Attachment #389200 - Flags: review?(dkl) → review+
(Assignee)

Comment 16

8 years ago
(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.
Flags: approval3.4+
Flags: approval+
(Assignee)

Updated

8 years ago
Attachment #389200 - Flags: review?(justdave)
(Assignee)

Comment 17

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: relnote
Resolution: --- → FIXED

Comment 18

8 years ago
Added to the 3.4.2 release notes in bug 515509.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.