Closed Bug 69000 Opened 24 years ago Closed 18 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: CodeMachine, Assigned: Wurblzap)

References

()

Details

(Keywords: selenium)

Attachments

(1 file, 21 obsolete files)

(deleted), patch
Wurblzap
: review+
Details | Diff | Splinter Review
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.
3.0: Need to consider if this will influence the schema even if not implemented.
Severity: normal → enhancement
Whiteboard: 3.0
Moving to real milestones...
Whiteboard: 3.0
Target Milestone: --- → Bugzilla 3.0
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.)
Assignee: tara → ian
Component: Bugzilla → Bugzilla 3
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.
Component: Bugzilla 3 → Bugzilla
Component: Bugzilla → Query/Bug List
Priority: -- → P4
Product: Webtools → Bugzilla
Version: other → unspecified
*** Bug 116173 has been marked as a duplicate of this bug. ***
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.
Attached patch Adds system/global queries restricted by group (obsolete) (deleted) — Splinter Review
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 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.
Attachment #62332 - Flags: review-
since there's a patch, disowning
Assignee: ian → zeroJ
*** Bug 143612 has been marked as a duplicate of this bug. ***
*** Bug 155326 has been marked as a duplicate of this bug. ***
*** Bug 178000 has been marked as a duplicate of this bug. ***
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 queryowner@mozilla.org 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.
Assignee: zeroJ → bugreport
Priority: P4 → P3
Summary: Group and system stored queries. → Permit a stored query to be marked "shared" and accessble by URL by other users
Target Milestone: Bugzilla 3.0 → Bugzilla 2.18
Comment on attachment 62332 [details] [diff] [review] Adds system/global queries restricted by group Marking prior patch obsolete
Attachment #62332 - Attachment is obsolete: true
Attached patch Patch (obsolete) (deleted) — Splinter Review
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.
Blocks: 178046
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
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
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..
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.
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.
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)
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.
>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.
Attachment #104927 - Flags: review?
Attached file Shared Queries Patch against 2.16.3 (obsolete) (deleted) —
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!
Attached file Shared Queries How To Document (obsolete) (deleted) —
This PDF describes how to use the Patch against 2.16.3 which is Attachment #128997 [details]
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?
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.
Attached patch Shared Queries Patch against 2.17.4 (obsolete) (deleted) — Splinter Review
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.
Attachment #128998 - Attachment mime type: application/octet-stream → application/pdf
Attachment #131119 - Flags: review?(bbaetz)
> +$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
Attached patch Patch agains 2.17 head of 30/09/2003 (obsolete) (deleted) — Splinter Review
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..
What's the status off this patch? Will it be committed? Is there anything that is holding it up?
Craig: it needs to be reviewed. So the author needs to ask someone to review it. :-) Gerv
Comment on attachment 104927 [details] [diff] [review] Patch Bitrotten.
Attachment #104927 - Flags: review? → review-
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
Gerv: Are you still refering to the 104927 patch which the previous comment was or is the latest patch 132411 also bitrotten?
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
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.
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.
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?
Oops, you're not copied here.
Attachment #131119 - Flags: review?(bbaetz)
Blocks: savedreports
No longer blocks: 178046
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Assignee: bugreport → nobody
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Attached patch Shared Queries for 2.19/HEAD patch (obsolete) (deleted) — Splinter Review
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.
Attachment #159694 - Attachment description: Saved Searches for 2.19/HEAD patch → Shared Queries for 2.19/HEAD patch
> This patch brings saved searches support sorry -- make that "shared queries", not "saved searches" -- it's too late :)
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.
*** Bug 262084 has been marked as a duplicate of this bug. ***
*** Bug 267750 has been marked as a duplicate of this bug. ***
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.
*** Bug 275759 has been marked as a duplicate of this bug. ***
*** Bug 255670 has been marked as a duplicate of this bug. ***
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).
Target Milestone: Bugzilla 2.22 → ---
Assignee: nobody → wurblzap
Attached patch Patch 2 (obsolete) (deleted) — Splinter Review
Allows sharing saved searches with a group. Sharing is being done on the Saved Searches user prefs panel.
Attachment #104927 - Attachment is obsolete: true
Attachment #128997 - Attachment is obsolete: true
Attachment #128998 - Attachment is obsolete: true
Attachment #131119 - Attachment is obsolete: true
Attachment #132411 - Attachment is obsolete: true
Attachment #139046 - Attachment is obsolete: true
Attachment #159694 - Attachment is obsolete: true
Attachment #222073 - Flags: review?
*** Bug 335456 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Summary: Permit a stored query to be marked "shared" and accessble by URL by other users → Permit a stored query to be marked "shared" and accessble by other users
Target Milestone: --- → Bugzilla 3.0
Target Milestone: Bugzilla 3.0 → Bugzilla 2.24
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 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
Attachment #222073 - Flags: review? → review-
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
(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'); }
Attached patch Patch 2.1 (obsolete) (deleted) — Splinter Review
Addressing comments. Thanks for the review!
Attachment #222073 - Attachment is obsolete: true
Attachment #224745 - Flags: review?(vladd)
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
Attachment #224745 - Flags: review?(vladd) → review-
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.
(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.
>> 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.
Attached patch Patch 2.2 (obsolete) (deleted) — Splinter Review
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.
Attachment #224745 - Attachment is obsolete: true
Attachment #224983 - Flags: review?(vladd)
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.
Attachment #224983 - Flags: review?(vladd) → review-
Attached patch Patch 2.3 (obsolete) (deleted) — Splinter Review
Addressing comments.
Attachment #224983 - Attachment is obsolete: true
Attachment #225274 - Flags: review?(vladd)
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?
Attachment #225274 - Flags: review?(vladd) → review-
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)
Attached patch Patch 2.4 (obsolete) (deleted) — Splinter Review
(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.
Attachment #225274 - Attachment is obsolete: true
Attachment #226213 - Flags: review?(vladd)
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.
Attachment #226213 - Flags: review?(vladd) → review-
Attached patch Patch 2.5 (obsolete) (deleted) — Splinter Review
(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.
Attachment #226213 - Attachment is obsolete: true
Attachment #226344 - Flags: review?(vladd)
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.
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.
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.
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.
(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).
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.
QA Contact: mattyt-bugzilla → default-qa
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.
Attached patch Patch 2.6 (obsolete) (deleted) — Splinter Review
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.
Attachment #226344 - Attachment is obsolete: true
Attachment #226845 - Flags: review?(vladd)
Attachment #226344 - Flags: review?(vladd)
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.
Attachment #226845 - Flags: review?(vladd) → review-
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.
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.
Attached patch Patch 2.6.1 (obsolete) (deleted) — Splinter Review
(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.
Attachment #226845 - Attachment is obsolete: true
Attachment #226988 - Flags: review?(vladd)
Summary: Permit a stored query to be marked "shared" and accessble by other users → Permit a stored query to be marked "shared" and accessible by other users
Attached patch Pg patch against 2.6.1 (obsolete) (deleted) — Splinter Review
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 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 wurblzap@gmail.com -- 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 :)
Attachment #226988 - Flags: review?(vladd) → review-
Attached patch Patch 2.6.2 (obsolete) (deleted) — Splinter Review
Did the Pg fix -- thanks for pre-testing it ;) Mentioning the querysharegroups param in the docs now, too.
Attachment #226988 - Attachment is obsolete: true
Attachment #226994 - Attachment is obsolete: true
Attachment #227151 - Flags: review?(vladd)
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.
Argh. Sorry. Tired.
Attached patch Patch 2.6.3 (obsolete) (deleted) — Splinter Review
Attachment #227151 - Attachment is obsolete: true
Attachment #227153 - Flags: review?(vladd)
Attachment #227151 - Flags: review?(vladd)
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 wurblzap@gmail.com -- 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.
Attachment #227153 - Flags: review-
(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.
(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.
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.
Attached patch Patch 2.6.4 (obsolete) (deleted) — Splinter Review
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.
Attachment #227153 - Attachment is obsolete: true
Attachment #227307 - Flags: review?(vladd)
Attachment #227153 - Flags: review?(vladd)
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 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)
Attachment #227307 - Flags: review?(vladd) → review+
Attached patch Patch 2.6.5 (obsolete) (deleted) — Splinter Review
Marc's patch with code changes updated to the tip.
Attachment #227307 - Attachment is obsolete: true
Attachment #229097 - Flags: review+
Flags: approval?
Flags: approval? → approval+
Attached patch Patch 2.6.5-Wurblzap (deleted) — Splinter Review
The patch I check in. A Param -> Bugzilla->params conversion in userprefs.cgi is missing from patch 2.6.5.
Attachment #229097 - Attachment is obsolete: true
Attachment #229149 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Marc, tboxes are now burning (doc only)
Attachment #229149 - Attachment description: Patch 2.6.5-wurblzap → Patch 2.6.5-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
Keywords: relnote
*** Bug 345767 has been marked as a duplicate of this bug. ***
Added to relnotes in bug 349423. Please let me know if I missed any critical information about this bug in the release notes.
Keywords: relnote
Flags: documentation?
Flags: testcase?
The original patch already updates the documentation. :)
Flags: documentation? → documentation+
Flags: testcase? → testcase+
Keywords: selenium
> 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 :)
ISTR it only works in boolean charts. Gerv
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: