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.

Wonderful that you attack the query editing, David! :)
I have also noticed a number of issues with it,
so any improvement is highly welcome.

Note that this fix is running in production on python.org (as well as my
own trackers, but python.org is public).

David, thanks again for the proposed improvement.

So your issue255081 is running along with the patch at these sides?

You mean the make-LinkHTTPProperty a new-style class?  Not currently,
but I could do that.

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).

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.

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?

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.

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.

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

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

pushed on changeset: 722394a48d7b and 1c90f15a177f

Also support work for a new cgi RestoreAction was added on 748ba87e1aca

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.