69000 - Permit a stored query to be marked "shared" and accessible by other users

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P3)

Description

[Matthew Tuck [:CodeMachine]]

You should be able to define stored queries for systems and groups rather than
just users.  This will save space in the database for commonly defined queries.

Comment 1

[Matthew Tuck [:CodeMachine]]

3.0: Need to consider if this will influence the schema even if not implemented.

Comment 2

[Dave Miller [:justdave]]

Moving to real milestones...

Comment 3

[Hixie (not reading bugmail)]

Taking all Bugzilla 3.0 bugs -- congratulations to MattyT for the triage, it
really was spot on. Note: I may end up pushing some of these bugs back to 3.2,
we'll see. However, I believe all these bugs should just fall out of the 
redesign. Let's hope I'm right!

(Note: I'm also resetting the priority field to "---" so that I can retriage any
that I consider important or likely to be dropped.)

Comment 4

[Dave Miller [:justdave]]

The Bugzilla 3 component is going away.  We're going to depend on the Milestones 
for this.  At the time this component was created, we didn't have milestones for 
Bugzilla.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 116173 has been marked as a duplicate of this bug. ***

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

While bug 116173 has a set of patches, I think they are needs-work and would
have marked them so.  I don't like the way those patches went about business
(Setting $userid to 0 seems flaky).  Also there is no reason to allow everyone
the ability to store global queries (which was spelled 'querys' in the patch). 
And if we can get code to do grouped queries as described here that would be
great too.

Comment 7

[John Vandenberg]

Attachment: Adds system/global queries restricted by group

The group storied queries should probably follow the change to the group system

(bug #68022).  This would also require a schema change as far as I can see.

I have locked down the system/global queries to the a specific group, however
the userid = 0 looks ok to me, as the only subsequent code uses it to place the
query in as userid 0, to which no user is created.  If it doesnt look ok, I can
just reproduce the code and hardcode the 0 into the SQL.

Comment 8

[Bradley Baetz (:bbaetz)]

Comment on attachment 62332 [details] [diff] [review]
Adds system/global queries restricted by group

The checksetup code shouldn't automatically add everyone to the system group.

Stored queries should use NULL, not 0, I think for the userid if you don't want
a separate table. However, a separate table would be good because then they
could be prefed (via another table) in teh same way the my bugs query is, so
that the user can show/hide them in the footer.

However, your patch seems to be missing new files, + changes to templates, so
you may have done some of that.

Comment 9

[Hixie (not reading bugmail)]

since there's a patch, disowning

Comment 10

[Bradley Baetz (:bbaetz)]

*** Bug 143612 has been marked as a duplicate of this bug. ***

Comment 11

[Bradley Baetz (:bbaetz)]

*** Bug 155326 has been marked as a duplicate of this bug. ***

Comment 12

[Bradley Baetz (:bbaetz)]

*** Bug 178000 has been marked as a duplicate of this bug. ***

Comment 13

[Joel Peshkin]

OK... if the other one is a duplicate of this apparrently abandoned bug, then
this one can become what can actually be done cleanly......



When a user creates a named query, another user should be able to perform a
query like.....

http://bugzilla.mozilla.org/buglist.cgi?namedcmd=queryname&user=queryowner%40mozilla.org

and run the query saved by [e-mail] canned "queryname"

When the query is executed, the results should be subject to the access rights
of the querying user, not the user defining the query.


------- Additional Comment #1 From Jeff Hedlund 2002-11-01 18:34 -------

I like this idea.  The user has to mark the query to be shared, right? 



Right.  I'm inclined to make this yet another option on creating a stored query.
On the named queries in the page footer, the user's own login name should be
placed in the link so that a user can copy the link location and send it to
another users.

Comment 14

[Joel Peshkin]

Comment on attachment 62332 [details] [diff] [review]
Adds system/global queries restricted by group

Marking prior patch obsolete

Comment 15

[Joel Peshkin]

Attachment: Patch

And, keeping it simple, here it is.

The links in the footer now include a field called "queryowner" with the login
name of the owner of the query.  When a named query is run, the queryowner is
used if it is differrent from the logged-in user, but requires that the query
be public.

Comment 16

[Gervase Markham [:gerv]]

As you may know, the next big item on my list of things to do after generic
reporting is generic charting. Design: http://www.mozilla.org.uk/temp/graph/ I
intend to start on this as soon as I've dealt with the few reporting bugs that
people have filed.

Because charts are resource-intensive things, I have designed a sharing
mechanism for them. And, as you can imagine, stored charts have quite a bit in
common with stored queries - the only difference being that charts are executed
periodically and the results stored.

So, part of the implementation would be to pull out the common function of the
two - and it shouldn't be too hard to make stored queries sharable and
subscribable-to as well as stored charts.

So, I would ask you to allow me to fix this bug in the course of that work.

Gerv

Comment 17

[Gervase Markham [:gerv]]

Aside from what I said before, I have several issues with this.

If you make queries publishable in this way, referenced by name, then you are
defining an interface. If person X publishes the URL to a query, they can then
never delete that query or change it, because if person Y has noticed the
published query and started using it, then it'll be extremely irritating for Y
to have the query disappear under his feet. You might say "but Y will want the
query 'updated'" - but how do you know that? You don't know who's using it.

Alternatively, if you say that you make it clear that queries are not guaranteed
to be permanent, then why not just publish the standard (long) query URL, which is?

In addition, this implementation makes the knob even more complicated, which is
a bad thing. (I am beginning to think that stored queries should be run from the
footer, and managed from user preferences, but that's not relevant here.)

Gerv

Comment 18

[Jussi Sirpoma]

My intrest in this bug is that I want to give beginning users some queries as a
starting point. This could be achieved by a similar mechanism as the MyBugs link
now. Some of the queries would be milestone specific and would therefore need
adjustment when the milestone is reached.

This could be a separate rfe, but the one I filed (bug 143612) and which was
marked as a duplicate of this one had the above idea. On the other hand, I'm not
that surpised that it was duped since the description of it seems to miss
several words..

Comment 19

[Joel Peshkin]

WRT comment 18, if you don't mind a bit of SQL hacking you could "copy" a set of
stored queries to each new user.

Comment 20

[Martin Ellison]

But you want team leaders to be able to create (and maintain) shared queries, 
so they can say, for example, that the Wednesday meeting discusses all the bugs 
matching the WednesdayMeeting shared query.

Comment 21

[Joel Peshkin]

Right.  And that is precisely the application.  

This actually looks like a popular enough idea that it is time to do more than
passively wait on it.   Gerv - what does it take to overcome your objections?

The objection in comment 17 is exactly why we DO want this feature rather than a
reason not wo want it.  And the addition to the knob is minucule and can even be
avoided.  (save first, control sharing from another page)

Comment 22

[Jouni Heikniemi]

I agree with Joel that this is a very needed feature, and especially the
WednesdayMeeting query example is a good one. That's what I need, too. 

Gerv sez:

>If you make queries publishable in this way, referenced by name, then you are
>defining an interface. If person X publishes the URL to a query, they can then
>never delete that query or change it, because if person Y has noticed the
>published query and started using it, then it'll be extremely irritating for Y
>to have the query disappear under his feet. You might say "but Y will want the
>query 'updated'" - but how do you know that? You don't know who's using it.

I agree that this is bad ui-wise, but the issue is mostly theoretic in the
environments that would benefit most from this. True, Y can get used to the
query and be harmed by the removal of it. On the other hand, it's also a matter
of internal Bugzilla policy. In bmo, this would suck. However, in a Bugzilla
environment used by a smallish group of developers (which may even have intense
live communication), there usually isn't a real social problem with removing
shared queries.

Comment 23

[Jussi Sirpoma]

>WRT comment 18, if you don't mind a bit of SQL hacking you could "copy" a set of
>stored queries to each new user.

Yes, that would do it for some queries. But as others have said here, the real
benefit would come from being able to maintain them.

If this would be done with some kind of subscription model, subscribed users
could be notified when the query has been changed/removed. The mail could have a
link to the query so that they could then create a normal private version from it.

This would not be the easiest way to achieve the "WednesdayMeeting" type of
queries, but could solve the mozilla.org case.

Anyway, simpler approach would do it for me.

Comment 24

[Saqib Rasul]

Attachment: Shared Queries Patch against 2.16.3

howdy! we deperatly had a need for shared queries, so we took the patch that
was already there and used that a starting point to develop the Shared Queries
system. It allows shared queries and the main features are:
- allow new and existing queries to be marked as shared
- users can run, load and make personal copies of shared queries
- users can put/remove shared queries in their page footer

The patch is against version 2.16.3 of Bugzilla. But it should work against
version 2.16 It works and is very useable and being used at my work place. A
PDF discribing how to use the system is will also be attached.

Hope it works for all you folks!

Comment 25

[Saqib Rasul]

Attachment: Shared Queries How To Document

This PDF describes how to use the Patch against 2.16.3 which is Attachment
#128997 [details]

Comment 26

[Dave Miller [:justdave]]

I think the feature list on the newly contributed patch would overcome Gerv's
fears (since it allows the user to make a personal copy of the query in their
own stored queries list if they care enough about it that they'll be hurt if it
disappears).  Unfortunately, since it's written against 2.16.3, and the
mechanism has changed so much since then, it'll likely take a lot of work to get
it working on the trunk so we can check it in... :(

Anyone want to take on porting this patch to the tip?

Comment 27

[Saqib Rasul]

Yes, we would be willing to port this patch over to 2.17.4 but we need an
indication if the features of the patch provided are worth porting over to the
2.17.4. So we need you to try out the patch. thereafter we need to get an
indication of how likely it is to be included into 2.17.4 and then eventually to
2.18. Once we get the OK from the people incharge we can start porting it.

Comment 28

[Saqib Rasul]

Attachment: Shared Queries Patch against 2.17.4

This patch has the same features as Patch 128997:
- allow new and existing queries to be marked as shared
- users can run, load and make personal copies of shared queries
- users can put/remove shared queries in their page footer

The patch is against version 2.17.4 of Bugzilla for the hope to be included in
the upcoming 2.18 version. At the moment with this patch you get shared
queries, however it would be good if anyone could modify it so that it is a
feature that can be enables and disables via the parameters.

Comment 29

[Gervase Markham [:gerv]]

> +$table{'shared_query_pref'} =

This rings warning bells :-) Why is there not a flag on the query table called
"public" or something like that?

Gerv

Comment 30

[Ludovic Dubost]

Attachment: Patch agains 2.17 head of 30/09/2003

This is a version of the patch with the same functionalities against the 2.17
Head code.. There has been a significant amount of code moved in User.pm and
changes needed to adapt to the new code..

Comment 31

[Craig Jeffree]

What's the status off this patch?  Will it be committed?  Is there anything that
is holding it up?

Comment 32

[Gervase Markham [:gerv]]

Craig: it needs to be reviewed. So the author needs to ask someone to review it. :-)

Gerv

Comment 33

[Jouni Heikniemi]

Comment on attachment 104927 [details] [diff] [review]
Patch

Bitrotten.

Comment 34

[Gervase Markham [:gerv]]

Yeah: this patch isn't just slightly bitrotten, it's a lot bitrotten, as I
rearranged the entire way stored queries are handled in the UI. In the new
world, the functions relating to a shared query are at the bottom of the results
page. And the "page footer" prefs panel is gone completely.

However, this approach has the as-yet-unfixed flaw that if your query causes an
error, you can't do anything with it. So here's what the right fix is:

Abstract out shared query operations into a separate strip on the query page,
visually separate and in a separate template. Include this template at that
point, but also in any error dialogs which might result from the shared query
being run. That fixes the above bug. It also gives a place to put a
"Share/Unshare this Search" button which the user can press if they are the
owner, or a "Add this to/Remove this from my list" button if they are not the owner.

Then, to share a query with someone, you send them the URL which invokes it
(which would contain an owner_id and a name) and they run it, and press the "Add
this to my list" button.

The database change would be a flag in the queries table saying "this is shared"
(settable and unsettable only by the owner) and then a user_query_map which
shows who has which queries in their shared queries.

The function of "make a copy" is served by clicking "Edit this Search",
rerunning it, and then using "Save Search As", as now.

Gerv

Comment 35

[Craig Jeffree]

Gerv: Are you still refering to the 104927 patch which the previous comment was
or is the latest patch 132411 also bitrotten?

Comment 36

[Gervase Markham [:gerv]]

Well, I didn't check the patch, but I read the document, and if the patch was
made before my big changes, it's definitely now broken :-)

Gerv

Comment 37

[Craig Jeffree]

Has anyone got the time to port this patch (132411) to the head of the CVS tree?
 Unfortunately I won't be able to get the time for quite a few months.  The
original author is also no longer able to.

Comment 38

[H.J. Lu]

Attachment: A patch for bugzilla-2.16.3 to fix the shared query

The shared query is useful. But the patch doesn't work for us.
The problem is the login ID is always used to query the database.
When you try to use other people's saved public queries, lookup
will always fail. This patch uses the query owner name if it is
available. It seems OK.

Comment 39

[Christian Reis]

Problem is I'd think it's hard to get this patch on 2.16 unless there's a really
good reason to, given that it's über-stable at this point. 

H.J., how important do you reckon it would be to have this feature in the
official release?

Comment 40

[Christian Reis]

Oops, you're not copied here. 

Comment 41

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 42

[Mike Schrag]

Attachment: Shared Queries for 2.19/HEAD patch

This patch brings saved searches support up to the 2.19 version on HEAD.  It's
a rather large change on the UI side.  This extends the view of saved searches
on the Saved Searches prefs tab by adding the owner name and a checkbox for
making your saved searches public/private.

Additionally, I added a small change where in your query you can use %userid%
that will be dynamically replaced with the executing user's userid.  For
instance, if you select "Owner" "is" "%userid" in the search page and save it
as public, whoever runs the query will see their own bugs.

Comment 43

[Mike Schrag]

> This patch brings saved searches support 
sorry -- make that "shared queries", not "saved searches" -- it's too late :)

Comment 44

[Joel Peshkin]

Comment on attachment 159694 [details] [diff] [review]
Shared Queries for 2.19/HEAD patch

I dont think we want every user to be able to create totally public searches. 
From what I read so far, it looks like this is not really taken into account. 
Please add a comment on this bug explaining the logic.	Also, bug 244239
already added user and group pronouns (like %user%) to Search.pm's boolean
charts.  If you want that extended, it should be a patch to Search.pm, not a
hack in the UI.

Comment 45

[Dave Miller [:justdave]]

*** Bug 262084 has been marked as a duplicate of this bug. ***

Comment 46

[Jussi Sirpoma]

*** Bug 267750 has been marked as a duplicate of this bug. ***

Comment 47

[Stuart Donaldson]

This looks pretty useful, but it seems to be missing a couple of things that
could really help.

a) The ability to apply the shared query to the new reporting/charting series
database.  Or is this part of the 2.19 feature set already?

b) The ability to have parameterized queries, at least based on product.

Where shared or public queries are concerned, there is a compelling reason to
want them applicable across multiple products.  In order to do this, an
interface which allows selecting a product, and a query to apply to the product
would be useful.

Comment 48

[Dave Miller [:justdave]]

*** Bug 275759 has been marked as a duplicate of this bug. ***

Comment 49

[Vlad Dascalu]

*** Bug 255670 has been marked as a duplicate of this bug. ***

Comment 50

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Only bug fixes are accepted, no enhancement bugs. As this bug has a pretty low activity (especially from the assignee), it's retargetted to ---. If you want to work on it and you think you can have it fixed for 2.24, please retarget it accordingly (to 2.24).

Comment 51

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2

Allows sharing saved searches with a group. Sharing is being done on the Saved Searches user prefs panel.

Comment 52

[Max Kanat-Alexander]

*** Bug 335456 has been marked as a duplicate of this bug. ***

Comment 53

[hacksaw]

I really hope folks will support this enhancement. Trying to send a reminder to developers to go look at their priority 1 bugs when we are coming up to an important release date is made harder due to the gigantic URL that such things generate. 8 lines in my mail reader, which of course "conveniently" broke the lines for me.

Comment 54

[Vlad Dascalu]

Comment on attachment 222073 [details] [diff] [review]
Patch 2

Sorry for the delay.

I'm getting an SQL error after applying the patch on the trunk and running checksetup.pl again, and then visiting the BZ homepage.

It seems you generate an empty list for the IN clause due to the User.pm code:

Template->process() failed twice.
First error: undef error - DBD::mysql::db selectall_arrayref failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') GROUP BY nq.id ORDER BY UPPER(name)' at line 12 [for Statement " SELECT nq.id, userid, name, query, query_type, nqgm.group_id AS shared_with_group, COUNT(nql.namedquery_id) AS linkinfooter FROM namedqueries AS nq LEFT JOIN namedquery_group_map nqgm ON nqgm.namedquery_id = nq.id LEFT JOIN namedqueries_linkinfooter AS nql ON nql.namedquery_id = nq.id AND nql.user_id = ? WHERE nq.userid = ? OR nqgm.group_id IN () GROUP BY nq.id ORDER BY UPPER(name)"] at Bugzilla/User.pm line 214

The other one is basically the same thing:

Template->process() failed twice.
First error: undef error - DBD::mysql::db selectall_arrayref failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') GROUP BY nq.id ORDER BY UPPER(name)' at line 12 [for Statement " SELECT nq.id, userid, name, query, query_type, nqgm.group_id AS shared_with_group, COUNT(nql.namedquery_id) AS linkinfooter FROM namedqueries AS nq LEFT JOIN namedquery_group_map nqgm ON nqgm.namedquery_id = nq.id LEFT JOIN namedqueries_linkinfooter AS nql ON nql.namedquery_id = nq.id AND nql.user_id = ? WHERE nq.userid = ? OR nqgm.group_id IN () GROUP BY nq.id ORDER BY UPPER(name)"] at Bugzilla/User.pm line 214

Comment 55

[Vlad Dascalu]

Oh, maybe this helps in case it's one of those MySQL version-dependent thing:

[vladd@abacron-l Bugzilla]$ mysql --version
mysql  Ver 14.12 Distrib 5.0.21, for redhat-linux-gnu (i386) using readline 5.0
[vladd@abacron-l Bugzilla]$ mysql
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 29 to server version: 5.0.21

Type 'help;' or '\h' for help. Type '\c' to clear the buffer.

mysql> select '3' in ('3');
+--------------+
| '3' in ('3') |
+--------------+
|            1 |
+--------------+
1 row in set (0.00 sec)

mysql> select '3' in ();
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1

Comment 56

[Joel Peshkin]

(In reply to comment #55)

> mysql> select '3' in ();

This is why the original code added a '-1' to all of the group lists.  That would keep the list from ever being empty.
 for example....

sub groups_as_string {
    my $self = shift;
    return (join(',',values(%{$self->groups})) || '-1');
}

Comment 57

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.1

Addressing comments.

Thanks for the review!

Comment 58

[Vlad Dascalu]

Comment on attachment 224745 [details] [diff] [review]
Patch 2.1

I'm now getting "IN (, NULL)":

undef error - DBD::mysql::db selectall_arrayref failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ' NULL) GROUP BY nq.id ORDER BY UPPER(name)' at line 12 [for Statement " SELECT nq.id, userid, name, query, query_type, nqgm.group_id AS shared_with_group, COUNT(nql.namedquery_id) AS linkinfooter FROM namedqueries AS nq LEFT JOIN namedquery_group_map nqgm ON nqgm.namedquery_id = nq.id LEFT JOIN namedqueries_linkinfooter AS nql ON nql.namedquery_id = nq.id AND nql.user_id = ? WHERE nq.userid = ? OR nqgm.group_id IN (, NULL) GROUP BY nq.id ORDER BY UPPER(name)"] at Bugzilla/User.pm line 216 Bugzilla::User::queries('Bugzilla::User=HASH(0xa464658)') called at data/template/template/en/default/global/site-navigation.html.tmpl line 154

Comment 59

[Joel Peshkin]

You need to put the logic inside the routine like....

+sub queryshare_groups_as_string {
+    my $self = shift;
+    return join(', ', (-1, @{$self->queryshare_groups()}));
+}
+

I don't think you want to use NULL either.

Comment 60

[Marc Schumann [:Wurblzap]]

(In reply to comment #58)
> I'm now getting "IN (, NULL)":

* smacks self *

(In reply to comment #59)
I've been following bug 284264 comment 3 here, asking not to include -1 in the list... NULL seems to work for me, why not NULL? And should NULL be allowed to be used, I'm ready and willing to change queryshare_groups accordingly.

Comment 61

[Vlad Dascalu]

>> why not NULL?

I think joel was talking about http://dev.mysql.com/doc/refman/5.0/en/working-with-null.html

>> The NULL value can be surprising until you get used to it. Conceptually, NULL means “a missing unknown value” and it is treated somewhat differently from other values. To test for NULL, you cannot use the arithmetic comparison operators such as =, <, or <>.  <<

What we're using here is basically the "IN" operator. I've made some tests on MySQL 5.0 and I get different results for what should be the same result:

mysql> select 3 in (null);
+-------------+
| 3 in (null) |
+-------------+
|        NULL |
+-------------+
1 row in set (0.00 sec)

mysql> select 3 in (null, 4);
+----------------+
| 3 in (null, 4) |
+----------------+
|                |
+----------------+
1 row in set (0.00 sec)

Probably in the end we could make this work, but it usually gives strange results if you're not using the proper "IS NULL" operator.

Comment 62

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.2

Avoiding IN clauses altogether if the set might be empty.

Additional changes:
 - Some errors corrected (especially template variable filtering).
 - List of other users' shared queries gets a <none> marker now if empty.

Comment 63

[Vlad Dascalu]

Comment on attachment 224983 [details] [diff] [review]
Patch 2.2

Cool, I've played with it and it looks almost ready to get in. Nice job!

Here are some review notes, they are usual 1 minute fixes, except the last one which should be a 5 minute fix :)

-> some capitalization improvements in the Saved Searches pref panel: "Shared by" --> "Shared By", "Share with a group" as well needs capitalization fixes.


-> we have a backward interface for the new dorem stuff:
# Backwards-compatibility - the old interface had cmdtype="runnamed" to run
# a named command, and we can't break this because it's in bookmarks.
if ($cgi->param('cmdtype') eq "runnamed") {
    $cgi->param('cmdtype', "dorem");
    $cgi->param('remaction', "run");
}
however we should be consistent regarding the URL used, in order to be able to obtain the proper effect of the "visited" links. If I display a shared search in my footer, and then I visit the "Saved searches" pref tab, then I get 2 different URLs for running the query: the one in the tab is "http://localhost/Bugzilla-trunk/buglist.cgi?cmdtype=dorem&remaction=run&namedcmd=fuck&sharer_id=1" and the one in the footer is "http://localhost/Bugzilla-trunk/buglist.cgi?cmdtype=runnamed&namedcmd=fuck&sharer_id=1". So clicking one doesn't make the other one visited. We should be consistent (more, probably we should be consistent with the URL for private saved searches --meaning that probably we prefer the 2nd URL, but that's a nit).


-> +++ template/en/default/admin/groups/delete.html.tmpl	9 Jun 2006 08:27:30 -0000
@@ -92,6 +92,25 @@
     flag types from this group for me.</p>
   [% END %]
 
+  [% IF shared_queries %]
==> you need to add shared_queries to the interface of this template (and in other places where you modify the interface and you haven't updated it, if they exist)


-> you can end up having in the footer several queries with the same name (one that belongs to you, and others that are imported from other users and showned in the footer). They are functional since the URL for each one of them is different (due to the fact that it includes the sharer_id), but from an UI point of view there is no way to differenciate between them. So basically we end up having in the footer several different links with the same name. This is unacceptable and we need a solution. The simplest thing would be to say something in the lines of "error: you selected at least 2 queries with the same name to be shown in the footer" upon prefs submission. The other would be to add an UI indication in the footer (like the sharer's account name right after the search name: bugzilla_bugs bugzilla_bugs<vladd> bugzilla_bugs<myk> (and so on). Another fix would be to be able to use someone else's query under a different name. No matter what, we need a fix for this UI thing.


Oh, and of course, we'll need XML documentation for this feature (one or two paragraph), probably we can do without it on commit time but it would be nice to have.

Comment 64

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.3

Addressing comments.

Comment 65

[Vlad Dascalu]

Comment on attachment 225274 [details] [diff] [review]
Patch 2.3

+    my $sth_write = $dbh->prepare('INSERT INTO namedqueries_linkinfooter
+                                   (id, userid) VALUES (?, ?)');

exists despite having as definition:

+            namedqueries_linkinfooter_id_idx => {FIELDS => [qw(namedquery_id user_id)],

The column names don't match; probably that's why I'm getting:

<h1>Software error:</h1>
<pre>DBD::mysql::st execute failed: Unknown column 'id' in 'field list' [for Statement &quot;INSERT INTO namedqueries_linkinfooter
                                   (id, userid) VALUES (?, ?)&quot; with ParamValues: 1='1', 0='1'] at checksetup.pl line 4373

when running checksetup.pl with this patch applied and some named queries previously saved.

I've also noticed this, but it can be considered a nit:

Shareability is purely cosmetic. Is this intentional or is it a bug? I mean, if I bookmark a shared query URL, then I login as the author and select "Don't share", users that bookmarked it are still able to run it just fine. Shouldn't they get an error message or something like that?

Comment 66

[Vlad Dascalu]

Comment on attachment 225274 [details] [diff] [review]
Patch 2.3

+        # If this is the user's own query, remember information about it
+        # so that we may present editing links.
+        unless ($cgi->param('sharer_id')) {

Basically someone could announce the URL to a mailing list, and it would be nice if the author would see the edit options as well when he clicks on the link.

So instead of that "unless" condition, maybe we should compare it with the ID of the current user.

This is just something that I've thought and I needed to write down, I can live without it :) (but just mentioning for reference)

Comment 67

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.4

(In reply to comment #65 and comment #66)
> <pre>DBD::mysql::st execute failed: Unknown column 'id' in 'field list' [for

Whoa!
Fixed.

> Shareability is purely cosmetic. Is this intentional or is it a bug? I mean, if
> I bookmark a shared query URL, then I login as the author and select "Don't
> share", users that bookmarked it are still able to run it just fine. Shouldn't
> they get an error message or something like that?

Fixed.

> +        # If this is the user's own query, remember information about it
> +        # so that we may present editing links.
> +        unless ($cgi->param('sharer_id')) {
> 
> Basically someone could announce the URL to a mailing list, and it would be
> nice if the author would see the edit options as well when he clicks on the
> link.
> 
> So instead of that "unless" condition, maybe we should compare it with the ID
> of the current user.

Misleading comment reworded. The "Edit" link is displayed in any case. The "unless" condition takes care that the query owner gets the query name field pre-filled so that the modified query can be easily saved under the original name.

Comment 68

[Vlad Dascalu]

Comment on attachment 226213 [details] [diff] [review]
Patch 2.4

I can't perform basic testing with this version. I keep getting the access denied screen when I try to run a query from my other account.

I've looked at the code and maybe the mistake is around this area:

+        if (!grep {$_ == $id} values(%{$user->groups()})) {
+            ThrowUserError("missing_query", {'queryname' => $name,
+                                             'sharer_id' => $sharer_id});
+        }

As I understand things, shouldn't that $id be $group instead?


In reply to the unless nit:

My point was that if I have an user id, let's call it $x, then I should always get the same results by adding:

&sharer_id=$x

to the URL. Due to the fact that my boss for example could have posted the link to my shared query on a mailing link, I should not get different results when I click on that link in the mailing list, which ends in &sharer_id=$x (compared to clicking the query in the Bugzilla interface). So all those places where we do unless ($sharer_id), or if ($sharer_id), could probably be improved in order to compare $sharer_id with the current id of the logged user (and continue on that path only if they are different). But again all this stuff is optional and I say IMO we can do without it if you don't want to implement it in this bug.

Comment 69

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.5

(In reply to comment #68)
> As I understand things, shouldn't that $id be $group instead?

Right. Fixed.

> In reply to the unless nit:

Ok, I misunderstood.
Fixed.

Comment 70

[Vlad Dascalu]

Comment on attachment 226344 [details] [diff] [review]
Patch 2.5

There is a landfill Bugzilla available with this patch at:

http://landfill.mozilla.org/bz69000/

Myk reviewed the UI and suggested the following:

 1. Seems like it would make more sense to put the name of the user who is sharing the bookmark into a tooltip rather than next to the link since most of the time folks aren't going to want that information.

 ( people who share searches are working together, so they're likely to be able to work out the conflicts by renaming their shared searches )

 2. i understand that it would make the code more complicated, but it'd make sense to say here "The search named world domination is no longer being shared with you." or the like. To differentiate between "it was shared with you, but now it's not" and "it was never shared with you"

 3.  Seems like folks in multiple groups are going to want to share searches with multiple groups.

 4.  I suspect that the number one feature users at Mozilla anyway are going to request is to be able to automatically "show in footer" any searches made available by a certain user or group.  So that f.e. everyone working on Firefox 2 can join a group firefox2devs and then automatically get the searches relevant to the current stage of the development process.

After asking him what should be upon the initial commit and what should be in follow-up bugs, myk said that one useful criteria that we could apply is " how might a user interact usefully with such a feature?" (basically -- what b.m.o. users want, and how to provide them with the best possible UI).

We also defined a user case:

right now the bugzilla development team shares searches of interest to the team by creating a tinyurl representing the search and putting it into the topic of #mozwebtools

 they would like a better way to share that search which

1. makes it available within the bugzilla interface itself, so team members don't have to log onto IRC to get the URL

2. is updated automatically when search parameters change

3. can be managed by one member (or multiple members?) of the team

4. is removed automatically when the search is no longer relevant

5. is added automatically when a new search becomes relevant

So, if the answer to the question in #3 is one member, then it's sufficient for everyone to watch one person's query (i.e. you don't need to establish the concept of watching a group and having a member of the group by the group admin) 

If #5 is desired functionality, then you want some way to have people subscribe to other people/groups rather than specific searches.


So Marc, basically you could look over this stuff, and decide what goes in the first-commit train and what doesn't (based on the experience of the Bugzilla users and stuff).

That looks like the last stage before checking this in.

Comment 71

[Vlad Dascalu]

Oh, I forgot, thanks to LpSolit for setting up the demo landfill installation, and to myk for the comments that I summarized in comment 70.

Comment 72

[Frédéric Buclin]

I played with the installation on landfill a bit yesterday, and here are my thoughts:

1. When you create a new saved search, it should be automatically added to the footer. This is the current behavior and we shouldn't revert this. If first thought I did something wrong when I didn't see it in the footer.

2. Maybe should we separate *my* own queries from shared queries offered by others.

3. There should probably be a "bz_can_share_queries" group, and it would required that you have these privs to be allowed to share your queries. I wouldn't want to be "spammed" by some &*%ç&* user's queries, despite I don't care if they can use our owns.

4. Could it be possible for shared queries to have a description field where the user making it public can explain what the query is supposed to do? Yesterday, I had to edit each shared query to understand what they did exactly. That was pretty annoying.

5. I like this feature :)

These are only my thoughts. Do what you want with them.

Comment 73

[Max Kanat-Alexander]

FWIW, when I was going to implement this, I was going to make people be able to share queries with any group that they could bless, and then those queries would show up automatically on that group's footer unless the individual removed it from the footer. That's much more useful in corporate environments, where people don't want to depend on you "subscribing" to the saved search.

Comment 74

[Marc Schumann [:Wurblzap]]

(In reply to comment #73)

I considered your suggestion to allow blessers to share their queries from your earlier comment on this bug, but this would restrict sharing to those who may bless, and I think this would be a restriction which is not easily explained... I put a comment into User.pm how mentioning the idea, though (minus auto-subscription).

Comment 75

[alanjstr]

Might I suggest that the handling of the footer be put into another bug.  That will let the core functionality of this one land.  There are several ideas and they may depend on the bugzilla installation as to how users want it to behave.

Comment 76

[Matt England]

I tried to establish an account at:
http://landfill.mozilla.org/bz69000/
with no success.  Is this landfill space open to anyone/everyone?  I was interested in trying out the new UI.  I've been waiting for this capability for some time.

Forgive me if this is an inappropriate comment.

Comment 77

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.6

Thanks for reviews and valuable comments! Here's as far as I think I'll get.

(In reply to comment #70)
>  1. Seems like it would make more sense to put the name of the user who is
> sharing the bookmark into a tooltip rather than next to the link since most of
> the time folks aren't going to want that information.

Done, although I liked it better before.

>  2. i understand that it would make the code more complicated, but it'd make
> sense to say here "The search named world domination is no longer being shared
> with you." or the like. To differentiate between "it was shared with you, but
> now it's not" and "it was never shared with you"

It may be only me, but this seems to me not to be worth the effort.

>  3.  Seems like folks in multiple groups are going to want to share searches
> with multiple groups.

Let's add this later.

>  4.  I suspect that the number one feature users at Mozilla anyway are going to
> request is to be able to automatically "show in footer" any searches made
> available by a certain user or group.  So that f.e. everyone working on Firefox
> 2 can join a group firefox2devs and then automatically get the searches
> relevant to the current stage of the development process.

If I share a query with a group I may bless, then all direct members of this group now get automatically subscribed to the query. This is a compromise with Max's suggestion.

> 1. makes it available within the bugzilla interface itself, so team members
> don't have to log onto IRC to get the URL

Check.

> 2. is updated automatically when search parameters change

Check.

> 3. can be managed by one member (or multiple members?) of the team

One member. (Or rather, one Bugzilla account.)

> 4. is removed automatically when the search is no longer relevant

Check.

> 5. is added automatically when a new search becomes relevant

Semi-check -- depends on proper group definitions and memberships.

(In reply to comment #72)
> 1. When you create a new saved search, it should be automatically added to the
> footer. This is the current behavior and we shouldn't revert this. If first
> thought I did something wrong when I didn't see it in the footer.

Fixed.

> 2. Maybe should we separate *my* own queries from shared queries offered by
> others.

Added <br>.

> 3. There should probably be a "bz_can_share_queries" group, and it would
> required that you have these privs to be allowed to share your queries. I
> wouldn't want to be "spammed" by some &*%ç&* user's queries, despite I don't
> care if they can use our owns.

Added querysharegroup param.

> 4. Could it be possible for shared queries to have a description field where
> the user making it public can explain what the query is supposed to do?
> Yesterday, I had to edit each shared query to understand what they did exactly.
> That was pretty annoying.

Good idea. New bug, please.

(In reply to comment #76)
See http://www.bugzilla.org/support/, Matt. I'd try the IRC channel to find help on this.

Comment 78

[Vlad Dascalu]

Comment on attachment 226845 [details] [diff] [review]
Patch 2.6

Thank you Marc!

We crash on Postgresql with this patch.

Postgres currently implements the SQL92 definition, which is that you can't refer to an ungrouped column except within an aggregate function call.

So basically, if you do group by on a column, Postgresql is not smart enough to realize whether the column is unique or not (a primary key or something like that), and then it complains if you use other columns of the same table without group by or aggregate functions.

The index.cgi page generates for me, with Postgresql and the path applied:

Template->process() failed twice.

 First error: undef error - DBD::Pg::db selectall_arrayref failed: ERROR: column "nq.userid" must appear in the GROUP BY clause or be used in an aggregate function [for Statement " SELECT nq.id, userid, name, query, query_type, nqgm.group_id AS shared_with_group, COUNT(nql.namedquery_id) AS linkinfooter FROM namedqueries AS nq LEFT JOIN namedquery_group_map nqgm ON nqgm.namedquery_id = nq.id LEFT JOIN namedqueries_linkinfooter AS nql ON nql.namedquery_id = nq.id AND nql.user_id = ? WHERE nq.userid = ? OR nqgm.group_id IN (3,13,7,10,5,12,1,8,11,4,2,6,9) GROUP BY nq.id, name, query, query_type ORDER BY UPPER(name)"] at Bugzilla/User.pm line 223

 Second error: undef error - DBD::Pg::db selectall_arrayref failed: ERROR: column "nq.userid" must appear in the GROUP BY clause or be used in an aggregate function [for Statement " SELECT nq.id, userid, name, query, query_type, nqgm.group_id AS shared_with_group, COUNT(nql.namedquery_id) AS linkinfooter FROM namedqueries AS nq LEFT JOIN namedquery_group_map nqgm ON nqgm.namedquery_id = nq.id LEFT JOIN namedqueries_linkinfooter AS nql ON nql.namedquery_id = nq.id AND nql.user_id = ? WHERE nq.userid = ? OR nqgm.group_id IN (3,13,7,10,5,12,1,8,11,4,2,6,9) GROUP BY nq.id, name, query, query_type ORDER BY UPPER(name)"] at Bugzilla/User.pm line 223

SQL99 solves that, but since Postgresql still follows SQL92, I think we can hack it a bit. Since we know the value is unique, probably all we need is to use MAX or something like that on that column.

More information here:
http://archives.postgresql.org/pgsql-general/2004-02/msg01214.php
(contains the answer but read the quoted text to understand the context, which is really our problem in a simplier test-case).

If it weren't for Pg this would be a review+, really nice.


Nit: can we add in the xml docs patch one sentence about automatic footer inclusion when the author shares something and he's able to bless a bug? We can leave this for later, so request documentation? if the sentece doesn't make it in the next patch.

Comment 79

[Vlad Dascalu]

Oh, by the way:

http://sqlzoo.net/howto/source/z.dir/err934/postgres

Due to this, if we wrap that column in a MAX(...), then that WHERE condition will have to be moved after the "GROUP BY" and turned into "HAVING" due to that error.

Comment 80

[Vlad Dascalu]

Another way to avoid the MAX hack and to keep the SQL sane would be to use the DISTINCT operator on the (.id, .userid) pair and select that (SELECT DISTINCT ...). This is what:

http://archives.postgresql.org/pgsql-general/2004-02/msg01193.php

suggests:

>> Since you're not agregating data, can't you use a select distinct instead? <<

Not sure if it's applicable to our case, but mentioning it for reference.

Comment 81

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.6.1

(In reply to comment #78)
> We crash on Postgresql with this patch.

Fixed, I think (can't test on Pg). Seems still to work on MySQL, though.

> Nit: can we add in the xml docs patch one sentence about automatic footer
> inclusion when the author shares something and he's able to bless a bug? We can
> leave this for later, so request documentation? if the sentece doesn't make it
> in the next patch.

Done.

Comment 82

[Vlad Dascalu]

Attachment: Pg patch against 2.6.1

This is the patch that I had to apply to 2.6.1 in order to stop PG from complaining.

Please review if the changes are sound, I've spend half an hour because I've done s/userid/MAX(userid)/ instead of s/userid/MAX(userid) AS userid/  :-)

There's also a mistake in patch 2.6.1 that I'll describe shortly in the next comment.

Comment 83

[Vlad Dascalu]

Comment on attachment 226988 [details] [diff] [review]
Patch 2.6.1

This still fails under Pg. I've added an attachment in my previous comment that should fix all Pg crashes. It still needs to be checked it's correct and doesn't alter the SQL logic (I assume it is).

+                     undef, ($dbh->bz_last_key, $userid));

This is incorrect per the definition in DB/Pg.pm it wants 2 parameters: the table name and the column name.

+# 2006-06-19 [e-mail] -- Bug 69000

This used to be 2006-06-24 in the previous version, so now it's going backwards :). Anyway, hopefully someone will remember to fix it the day this gets checked in.

There is a severe bug/regression in the groups system that prevents me to fully enjoy/test this, and I'm not up to speed with groups to check this fully. Either that or I've messed up my BZ install. I'll try to digg it up. One question: what is exactly the definition of visible_groups_inherited? (in natural language, English I mean :) ). I mean, what is it supposed to return? And what if a user is both inherits and is explicitly a member of a group, should that still work?

What's up with the querysharegroup param? You haven't said a thing about it in the comments, or I skipped that part. Another sentence in the docs about it would be nice. :)

Hopefully, if your review of my Pg patch is ok, the next version should be the last :)

Comment 84

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.6.2

Did the Pg fix -- thanks for pre-testing it ;)

Mentioning the querysharegroups param in the docs now, too.

Comment 85

[Vlad Dascalu]

Marc: you still have

+                     undef, ($dbh->bz_last_key, $userid));

I've said in a previous comment that bz_last_key requires two parameters, the table and the column name. I guess it's something that we can fix on checkin, but a patch with it fixed would have been nice.

Comment 86

[Marc Schumann [:Wurblzap]]

Argh.
Sorry.
Tired.

Comment 87

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.6.3

Comment 88

[Max Kanat-Alexander]

Comment on attachment 227153 [details] [diff] [review]
Patch 2.6.3

> sub LookupNamedQuery {

  This might be a good time to move this code into User.pm.

  In general, I think this patch would be hugely enhanced and made much less complex by just modifying the "queries" property of User.pm, or by adding a new property to help out.

  You could even have an update_named_queries function in User.pm, too, perhaps, that took an array of hashes or an array of Bugzilla::SavedSearch objects (which don't exist yet, I think). It would be pretty easy to create the object with Bugzilla::Object.

>+# 2006-06-27 [e-mail] -- Bug 69000
>+$dbh->bz_add_column('namedqueries', 'id',
>+                    {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
>+if ($dbh->bz_column_info("namedqueries", "linkinfooter")) {
>+    # Move linkinfooter information into a table of its own.

  If we're going to move it and rename it, the new table name should probably conform to our modern_naming_guidelines, meaning that it gets underscores_between_every_word.

>+    my $sth_read = $dbh->prepare('SELECT id, userid
>+                                    FROM namedqueries 
>+                                   WHERE linkinfooter = 1');
>+    my $sth_write = $dbh->prepare('INSERT INTO namedqueries_linkinfooter
>+                                   (namedquery_id, user_id) VALUES (?, ?)');

  You can also do an INSERT INTO ( ) SELECT FROM here.

>--- userprefs.cgi	21 Jun 2006 00:44:48 -0000	1.99
>+++ userprefs.cgi	26 Jun 2006 22:32:13 -0000
>@@ -350,11 +350,12 @@
>         my ($nam, $desc) = @$group;
>         push(@has_bits, {"desc" => $desc, "name" => $nam});
>     }
>-    $groups = $dbh->selectall_arrayref(
>-                "SELECT DISTINCT name, description FROM groups ORDER BY name");
>+    $groups = $dbh->selectall_arrayref('SELECT DISTINCT id, name, description
>+                                          FROM groups
>+                                         ORDER BY name');
>     foreach my $group (@$groups) {
>-        my ($nam, $desc) = @$group;
>-        if ($user->can_bless($nam)) {
>+        my ($group_id, $nam, $desc) = @$group;
>+        if ($user->can_bless($group_id)) {

  Hrm...is this a fix for some other bug? It's okay, I'm just wondering.

>@@ -397,18 +405,112 @@
>     my $dbh = Bugzilla->dbh;
>     my $user = Bugzilla->user;
> 
>+    # We'll need this in a loop, so do the call once.
>+    my $user_id = $user->id;
>+

  Calling $user->id is basically instantaneous, FWIW. But if this makes the code more readable, it's fine.

>     my @queries = @{$user->queries};
>-    my $sth = $dbh->prepare("UPDATE namedqueries SET linkinfooter = ?
>-                          WHERE userid = ?
>-                          AND name = ?");
>+    my $sth_check_nl = $dbh->prepare('SELECT COUNT(*)
>+                                        FROM namedqueries_linkinfooter
>+                                       WHERE namedquery_id = ?
> [snip]

  Some of the code in this section I'd really like to see in a module, if possible.

>+                    # If we're sharing our query with a group we can bless,
>+                    # we're subscribing direct group members to our search
>+                    # automatically. Otherwise, the group members need to
>+                    # opt in. This behaviour is deemed most likely to fit
>+                    # users' needs.

  Cool. That's fine, as long as it gets explained to the user on the result page.

>Index: Bugzilla/User.pm

>+    my $queries_ref = $dbh->selectall_arrayref('
>+                    SELECT nq.id, MAX(userid) AS userid, name, query, query_type,
>+                           MAX(nqgm.group_id) AS shared_with_group,
>+                           COUNT(nql.namedquery_id) AS linkinfooter

  This would be particularly something that should be inside of another method. (That is, queries property of User.pm, or something like a "queries_available" property.)

>+                {'Slice'=>{}}, $self->{id}, $self->{id});

  We usually try to use $self->id always, even inside the module itself. It makes it easier if some day we need to modify the accessor (sub id {}) to do something different than normal.

>+
>+    foreach my $queries_hash (@$queries_ref) {
>+        # For each query, determine whether it's being used in a whine.
>+        if (exists($$used_in_whine_ref{$queries_hash->{'name'}})) {

  This is a good example of a place where it would be much better to have a Bugzilla::SavedSearch object. It would also avoid writing a lot of different and customized SQL in various places.

>Index: Bugzilla/Config/GroupSecurity.pm
> [snip]
>   {
>+   name => 'querysharegroup',

  Why not just create a bz_can_share_queries group instead? I think that's more flexible and easily understandable to the end user. Most of the users I've run into don't really understand the parameter-group things.

>Index: Bugzilla/DB/Schema.pm
>+    namedquery_group_map => {
>+        FIELDS => [
>+            namedquery_id => {TYPE => 'INT3', NOTNULL => 1},
>+            group_id      => {TYPE => 'INT3', NOTNULL => 1},
>+        ],
>+        INDEXES => [
>+            namedquery_group_map_namedquery_id_idx   =>
>+                {FIELDS => [qw(namedquery_id)], TYPE => 'UNIQUE'},

  This is the reason for the r-:  It looks like that index should be on both fields, yes?

>Index: Bugzilla/DB/Schema/Mysql.pm
> [snip]
>     rep_platform   => {isactive => 1},
>     op_sys         => {isactive => 1},
>     profiles       => {mybugslink => 1, newemailtech => 1},
>-    namedqueries   => {linkinfooter => 1, watchfordiffs => 1},

  No, don't edit this file. This file is a historical reference of every field that ever *was* a boolean in Bugzilla. I'm pretty sure the comment at the top says that. If it doesn't, I should make it clearer.


>Index: docs/xml/using.xml
>+      On the Saved Searches tab of your User Preferences page (the Prefs link
>+      in Bugzilla's footer), members of the group defined in the
>+      querysharegroup parameter can share such Saved Searches with user groups
>+      so that other users may use them.

  It's a bit hard to wrap my mind around that. I know what it means, but I'm not sure a non-Bugzilla-expert would know. This is also where having a bz_can_share_queries group would make things easier to understand.

>+      At the same place, you can see Saved Searches other users are sharing and

  Add a comma after "sharing" to make the sentence easier to read.

>+      If somebody is sharing a Search with a group she or he is allowed to
>+      <xref linkend="groups">assign users to</a>, it will show up in the
>+      group's direct members' footers by default.

  I'm not sure that people know what "direct members" means. Do we explain that elsewhere?

>Index: template/en/default/global/user-error.html.tmpl
>+    [% docslinks = {'query.html' => "Searching for $terms.bugs",
>+                    'list.html'  => "$terms.Bug lists"} %]
>+    The search named <em>[% queryname FILTER html %]</em>
>+    [% IF sharer_id %]
>+      has not been made visible to you.
>+    [% ELSE %]
>+      does not exist.

  Should we really distinguish between those? I suppose that's not a dangerous information leak.

Comment 89

[Marc Schumann [:Wurblzap]]

(In reply to comment #88)
> > sub LookupNamedQuery {
> 
>   This might be a good time to move this code into User.pm.

Yes. This is a separate bug, though. All other modularization things you mentioned, too. This is because I don't want to touch existing functionality more than I need to.

> >+    # Move linkinfooter information into a table of its own.
> 
>   If we're going to move it and rename it, the new table name should probably
> conform to our modern_naming_guidelines, meaning that it gets
> underscores_between_every_word.

Makes sense to me. I'll do this.

>   Hrm...is this a fix for some other bug? It's okay, I'm just wondering.

I changed can_bless so that it operates on IDs instead of names.

> >+                    # If we're sharing our query with a group we can bless,
> >+                    # we're subscribing direct group members to our search
> >+                    # automatically. Otherwise, the group members need to
> >+                    # opt in. This behaviour is deemed most likely to fit
> >+                    # users' needs.
> 
>   Cool. That's fine, as long as it gets explained to the user on the result
> page.

New bug please. It's part of the docs already (part of the patch).

> >+                {'Slice'=>{}}, $self->{id}, $self->{id});
> 
>   We usually try to use $self->id always, even inside the module itself. It
> makes it easier if some day we need to modify the accessor (sub id {}) to do
> something different than normal.

Will do this.

> >+   name => 'querysharegroup',
> 
>   Why not just create a bz_can_share_queries group instead? I think that's more
> flexible and easily understandable to the end user. Most of the users I've run
> into don't really understand the parameter-group things.

I don't like system groups at all. I'd be happy if they'd be gone entirely at some time. This is because I think we should be consistent in what we do. What I mean is that products refer to any user-created group for group permissions. This is what global switches should do, too, imho.

>   This is the reason for the r-:  It looks like that index should be on both
> fields, yes?

No. Each named query may be shared with up to one group.

> >Index: Bugzilla/DB/Schema/Mysql.pm
>   No, don't edit this file. This file is a historical reference of every field

I at least didn't understand it. It says "This only has to be accurate up to and through 2.19.3", but it doesn't say you shouldn't touch it any more :)
I'll drop it from the next patch.

>   It's a bit hard to wrap my mind around that. I know what it means, but I'm
> not sure a non-Bugzilla-expert would know. This is also where having a
> bz_can_share_queries group would make things easier to understand.

Feel free to reword it in a patch on a new bug.

>   I'm not sure that people know what "direct members" means. Do we explain that
> elsewhere?

Well, direct is non-derived...
Same as above: feel free to reword it in a patch on a new bug.

Btw, I'd rather make this work for all members, but I can't because we can't find all members of a group (short of looking at each user individually and calling the flatten_group thingy). But this is a separate bug, too.

>   Should we really distinguish between those? I suppose that's not a dangerous
> information leak.

You get the "has not been made visible to you" message even if there is no such query at all. The message wording depends on whether you want to view your own or somebody else's query.

Comment 90

[Frédéric Buclin]

(In reply to comment #89)

> > >+   name => 'querysharegroup',
> > 
> >   Why not just create a bz_can_share_queries group instead?
> 
> I don't like system groups at all. I'd be happy if they'd be gone entirely at
> some time. This is because I think we should be consistent in what we do. What
> I mean is that products refer to any user-created group for group permissions.
> This is what global switches should do, too, imho.


I agree with Max about this point. I also much prefer a system group here.

Comment 91

[Vlad Dascalu]

I regard to comment #88 (mkanat):

Please fill new bugs for things that don't and shouldn't block this commit. We're trying to land this feature, so new bugs please :)

Thanks Max.

Comment 92

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.6.4

Patch which goes with comment 89.
Fixes a bug of Patch 2.6.3, too, which made own queries crop up in your own footer by way of the auto-subscription functionality.

Comment 93

[Vlad Dascalu]

Comment on attachment 227307 [details] [diff] [review]
Patch 2.6.4

Hi Marc,

I'm in a 4-day conference so the soonest that I'll reach to this is 10th of July.

If you think that you could hack something in the lines of a SavedSearch.pm Perl module until then, it would be great. Introducing additional SQL statements in .cgi scripts seems icky, probably we should really have a SavedSearch.pm perl module in the Bugzilla directory.

By no means is this issue blocking commit. I was just saying anyway that I can't make a review on this in the following 4-5 days, so if you have the time to move code in the .pm thing during this time interval, then it won't delay things more :)

Either case, I should be here on 10th, so see you then.

Comment 94

[Vlad Dascalu]

Comment on attachment 227307 [details] [diff] [review]
Patch 2.6.4

Looks good.

There are 2 changes that need to be performed in order to keep up with the recent changes.

One of them is the Param stuff (Bugzilla->param->{'param_name'} instead of the old method), and another one is a change in GroupSecurity.pm, in Config.

I'll post a patch with those two, carry the review and request approval.

(sorry for the delay)

Comment 95

[Vlad Dascalu]

Attachment: Patch 2.6.5

Marc's patch with code changes updated to the tip.

Comment 96

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.6.5-Wurblzap

The patch I check in. A Param -> Bugzilla->params conversion in userprefs.cgi is missing from patch 2.6.5.

Comment 97

[Marc Schumann [:Wurblzap]]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.340; previous revision: 1.339
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.504; previous revision: 1.503
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.76; previous revision: 1.75
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.126; previous revision: 1.125
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.119; previous revision: 1.118
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.103; previous revision: 1.102
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.116; previous revision: 1.115
done
Checking in Bugzilla/Config/GroupSecurity.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/GroupSecurity.pm,v  <--  GroupSecurity.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.53; previous revision: 1.52
done
Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.49; previous revision: 1.48
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.23; previous revision: 1.22
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.72; previous revision: 1.71
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v  <--  saved-searches.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/admin/groups/delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/delete.html.tmpl,v  <--  delete.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/admin/params/groupsecurity.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/groupsecurity.html.tmpl,v  <--  groupsecurity.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/admin/users/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v  <--  useful-links.html.tmpl
new revision: 1.46; previous revision: 1.45
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.176; previous revision: 1.175
done

Comment 98

[Frédéric Buclin]

Marc, tboxes are now burning (doc only)

Comment 99

[Marc Schumann [:Wurblzap]]

Emergency Docs Tinderbox fix checkin:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.50; previous revision: 1.49
done

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=using.xml&branch=&root=/cvsroot&subdir=mozilla/webtools/bugzilla/docs/xml&command=DIFF_FRAMESET&rev1=1.49&rev2=1.50

Comment 100

[Olav Vitters]

*** Bug 345767 has been marked as a duplicate of this bug. ***

Comment 101

[Max Kanat-Alexander]

Added to relnotes in bug 349423. Please let me know if I missed any critical information about this bug in the release notes.

Comment 102

[Frédéric Buclin]

The original patch already updates the documentation. :)

Comment 103

[Michael Shigorin]

> Additionally, I added a small change where in your query you can use %userid%
> that will be dynamically replaced with the executing user's userid.
Sorry but how to use this?  I've read the patch, grepped the sources and still when trying to substitute %userid% for assignee's email, the query error is:

  The name %userid% is not a valid username. Either you misspelled it,
  or the person has not registered for a Bugzilla account.

Otherwise, works just fine on 3.6 :)

Comment 104

[Gervase Markham [:gerv]]

ISTR it only works in boolean charts.

Gerv