Roundup Tracker - Issues

Issue 2550831

classification
Title: Make the classic template query.edit page work.
Type: behavior Severity: normal
Components: Web interface Versions:
process
Status: fixed Resolution: fixed
Dependencies: Superseder:
Assigned To: rouilj Nosy List: ber, r.david.murray, rouilj, schlatterbeck
Priority: Keywords: patch

Created on 2014-03-06 19:52 by r.david.murray, last changed 2016-07-04 02:51 by rouilj.

Files
File name Uploaded Description Edit Remove
query.edit.html.patch r.david.murray, 2014-03-06 19:52
Query_edit_non_admin.png rouilj, 2016-07-03 05:47
Query_edit_admin.png rouilj, 2016-07-03 05:56
Messages
msg4997 Author: [hidden] (r.david.murray) Date: 2014-03-06 19:52
The attached patch makes the classic template query.edit page work.  It
addresses several different problems, and makes one related content change:

  1) Previously, once a user included a public query that query would
show up with a yes/no dropdown in the private column.  When the form was
submitted, roundup would try to set the private_for property on that
query the user didn't own, and the result was a permission error on 'query'.

  2) The 'private' dropdown always showed 'yes', no matter what the
actual value of private_for was.  (See issue 2550830).

  3) Pressing the 'delete' button resulted in an invalid request error,
since it was doing a GET and roundup is requiring a POST.

The fix also comes in several parts:

  1) Restructure the loops so that we look at all queries and pick out
only the ones the user owns for the first section, and the ones they
don't for the second.

  2) Fix the private yes/no dropdown by using 'not query.private_for'

  3) Change the delete button javascript to use POST.

  4) Remove the retired queries section.  As written it would show only
retired queries that were included in the user's query list.  It seems
to me that when a user deletes a query, they want it gone, so we should
not show it, just as we don't show retired records in other interface
screens.  If it is desirable to keep it, it should be moved to the
bottom and fixed to show all retired queries created by the user.

Note that there are several things in this patch that perhaps could be
done better by someone with more knowledge of roundup internals.  For
example, I could not get filter to work, which is why I look at all
queries and use a condition to only include some of them.  (The fact
that filter arguments of private_for: uid and private_for: None returned
all queries may or may not be related to the LinkHTMLProperty issue). 
If we do need to iterate the whole list there may be a better way to get
it than calling filter with no arguments, I don't know.  Finally, I
don't know if there is a way to refer to query.is_retired without going
through the path expression.
msg5000 Author: [hidden] (ber) Date: 2014-03-07 08:07
Wonderful that you attack the query editing, David! :)
I have also noticed a number of issues with it,
so any improvement is highly welcome.
msg5001 Author: [hidden] (r.david.murray) Date: 2014-03-07 14:48
Note that this fix is running in production on python.org (as well as my
own trackers, but python.org is public).
msg5004 Author: [hidden] (ber) Date: 2014-03-10 10:27
David, thanks again for the proposed improvement.

So your issue255081 is running along with the patch at these sides?
msg5005 Author: [hidden] (r.david.murray) Date: 2014-03-10 10:41
You mean the make-LinkHTTPProperty a new-style class?  Not currently,
but I could do that.
msg5136 Author: [hidden] (ber) Date: 2014-09-02 13:35
Now that issue2550830 is resolved, I think we should evaluate if we want to 
apply this patch as well (or if it is still necessary).
msg5137 Author: [hidden] (r.david.murray) Date: 2014-09-02 14:30
This patch has a bug that I haven't gotten around to fixing yet.  If you
delete a query without first removing it from your included queries
list, it gets stuck in your included queries list.  That is presumably
the reason the deleted queries were originally included, so the fix to
my fix may just be to add that section back.
msg5551 Author: [hidden] (rouilj) Date: 2016-04-30 03:10
This seems to also address the problem raised in issue2550745
It also discusses trying to set the private_for value for the
public query which isn't allowed.


I am tempted to apply this patch and obsolete the patch in ..745
however I am not sure how to solve the bug R David reported
regarding the inability to remove a deleted query.

R David do you have a fix for this that you can upload?
msg5559 Author: [hidden] (r.david.murray) Date: 2016-05-02 16:19
Unfortunately no, I haven't gotten around to fixing it.  Which
presumably means I haven't run in to it on my own tracker yet :)

You could just add back the TAL section that lists the deleted queries.
 I'd prefer to automatically remove the query rather than do that, but
just adding the section back so the user can do it should be easier.
msg5725 Author: [hidden] (rouilj) Date: 2016-07-03 04:51
I finally have this patch in the pipeline.

I have chnaged the table layout a bit. I added section heads in the
table so it looks like (hope the ascii art works):

  Query     Include in "Your Queries"   Edit    Private to you?          
  *Queries I created*
  pede      include                     edit         yes          [Delete]
  edit      leave out                   edit         yes          [Delete]
  *Queries others created*
  clearcase leave out                   [not yours to edit]
  demo 1    include                     [not yours to edit]
  *My retired queries*
  alpha 2   [query is retired]                                    [Restore]
  [Save Selection]

I also added Restore action in cgi/Actions.py and added it to
client.py. To alow the user to restore queries, schema.py needs to
have:

  p = db.security.addPermission(name='Restore', klass='query',
check=edit_query,
      description="User is allowed to restore their queries")
  db.security.addPermissionToRole('User', p)

added to it.

Also R David's patch to LinkHTMLProperty looks like it solved the
filter issue. Note that filter() never returns retired issues, so I was
able to replace the tal:condition in all your tal:block and push it to
the filter using:

  <tr tal:define="queries python:db.query.filter(filterspec={'creator':
uid})"
    tal:repeat="query queries">
   <tal:block>

and

  <tr tal:define="queries
		python:db.query.filter(filterspec={'private_for': None})"
     tal:repeat="query queries">
   <tal:block>

This should help performance.


Hopefully I'll have it committed by Monday so people can kick the tires
on it.
msg5726 Author: [hidden] (rouilj) Date: 2016-07-03 05:56
Since a picture is worth 1k words, I attached two editing
scenarios: regular user and an administrative user.

The section "Queries I created" is the same for both.

For Queries created by others, the admin gets to edit the query
but can not make it private. We also list who owns the query.

For retired queries that are not in user/queries they just disappear.
If a active query you created is in your retired list, you get to
restore it. Then you can remove it from your list and re-retire if you want.

If the retired query is not yours, you can remove it from your active
list and it disappears.

Does that seem like a reasonable workflow?

-- rouilj
msg5727 Author: [hidden] (schlatterbeck) Date: 2016-07-03 14:33
On Sun, Jul 03, 2016 at 05:56:17AM +0000, John Rouillard wrote:
> 
> Since a picture is worth 1k words, I attached two editing
> scenarios: regular user and an administrative user.

[...]

> Does that seem like a reasonable workflow?

Looks very nice to me!

Ralf
msg5731 Author: [hidden] (rouilj) Date: 2016-07-03 23:33
pushed on changeset: 722394a48d7b and 1c90f15a177f

Also support work for a new cgi RestoreAction was added on 748ba87e1aca
msg5733 Author: [hidden] (rouilj) Date: 2016-07-04 02:51
Forgot one use case. A formerly public query is made private again.

when that happens there is now way to remove it from your query list.
I have modified the edit page to put them in the same
section as the retired queries. Committed as: 425b4c4fc345
along with a fix to security.py that adds the restore permission
to Admin role.

Opened issue2550915 to get a better solution in place.
History
Date User Action Args
2016-07-04 02:51:44rouiljsetmessages: + msg5733
2016-07-03 23:33:12rouiljsetstatus: open -> fixed
resolution: fixed
messages: + msg5731
2016-07-03 14:33:17schlatterbecksetmessages: + msg5727
2016-07-03 05:56:17rouiljsetfiles: + Query_edit_admin.png
messages: + msg5726
2016-07-03 05:47:38rouiljsetfiles: + Query_edit_non_admin.png
2016-07-03 04:51:34rouiljsetstatus: new -> open
messages: + msg5725
2016-05-02 16:19:07r.david.murraysetmessages: + msg5559
2016-04-30 03:10:01rouiljsetassignee: rouilj
messages: + msg5551
nosy: + rouilj
2014-09-02 14:30:20r.david.murraysetmessages: + msg5137
2014-09-02 13:35:31bersetnosy: + schlatterbeck
messages: + msg5136
2014-03-10 10:41:03r.david.murraysetmessages: + msg5005
2014-03-10 10:27:45bersetmessages: + msg5004
2014-03-07 14:48:08r.david.murraysetmessages: + msg5001
2014-03-07 08:07:29bersetnosy: + ber
messages: + msg5000
2014-03-06 19:52:06r.david.murraycreate