no apparent way to make set_owner default to existing owner

[matkinson@…]

We are using the custom workflow to manage our tickets and my users want the assignment of the owner to be separate from the promotion of state. For example, we have defined the Work state, which has TICKET_MODIFY permissions and set_owner operation.

Here is the problem we are seeing: The owner is assigned when the ticket is created (but sometimes, it may not be assigned until it is moved to Work). If the owner was assigned, then the radio dial that sets Work as the current state also has an owner field that defaults to the current editor, not the current owner. In order to keep the same owner, the editor has to cut and paste it. We would like a way to promote tickets without affecting the owner.

[Christian Boos]

So eventually have an action "may_set_owner"? PatchWelcome.

[anonymous]

Um I don't think it's added. At least not on 10.2

[anonymous]

oops I mean .12.2

[commerce@…]

Just wanting to "vote this up" by noting that my team is straining under the very same problem, and I found this ticket while Googling, hoping to find an existing solution. Although we're aware that the default set_owner is always the editor, we keep forgetting to change it, thus we keep "losing" ownership of tickets, the more we use custom ticket actions that have (otherwise very useful) set_owner operations.

[commerce@…]

Noting that #10463 is a duplicate. On that ticket, a developer responded to a similar request by closing the ticket and saying, essentially, "I don't believe that you need what you say you need." I think that was an inappropriate response, really, and have posted a follow-up comment over there to emphasize the reasonableness of the stated need, as my team has the very same need AND it seems rather obvious to us that it would come up again and again.

[ethan.jucovy@…]

Adds new workflow operation maybe_set_owner

[ethan.jucovy@…]

attachment:maybe_set_owner.patch​ implements this feature using a separate workflow operation as cboss suggested in comment:1. You would use it like:

reassign = new,assigned,accepted,reopened -> assigned
reassign.operations = maybe_set_owner
reassign.permissions = TICKET_MODIFY

[Remy Blank]

Good start. Note that Christian suggested may_set_owner, not maybe_set_owner, which is clearer IMO.

A few comments:

• You could simplify if 'set_owner' in operations or 'maybe_set_owner' in operations: as if operations in ('set_owner', 'may_set_owner'):.
• For default_owner, you could write default_owner = req.authname if operation == 'set_owner' else current_owner.
• I'm not sure you can use current_owner, as it will be set to (none) if the ticket has no owner (it's used for printing the message). You should probably use ticket['owner'] instead.

[ethan.jucovy@…]

Thanks for the code review and comments. I also noticed two other significant problems with my patch:

• When restrict_owner = true, the dropdown list only contains known users, so if the ticket currently has no owner then it would be impossible not to change the owner when using this operation.
• The operation doesn't actually do anything. :-) I neglected to make any changes to the get_ticket_changes method, so the operation is completely ignored.

For the first issue, would it make sense to inject a selected (none) option into the dropdown that's rendered for the may_set_owner operation if and only if the ticket currently has no owner? Or would it make more sense to always inject a selected (current owner) option into the dropdown for this operation, and special-case that logic? I'm leaning toward the latter, I think.

Later today I'll upload a new version of the patch that addresses your comments as well as these.

[Remy Blank]

Replying to ethan.jucovy@…:

For the first issue, would it make sense to inject a selected (none) option into the dropdown that's rendered for the may_set_owner operation if and only if the ticket currently has no owner? Or would it make more sense to always inject a selected (current owner) option into the dropdown for this operation, and special-case that logic? I'm leaning toward the latter, I think.

What we do in other parts of the code (e.g. when rendering a component drop-down) is to check if the current value is in the list, and if not, to add an element to the list with the current value. This makes sure it's always possible to leave the value unchanged.

[ethan.jucovy@…]

improved patch

[ethan.jucovy@…]

A new patch is here: attachment:may_set_owner.working.patch​ that addresses the above comments. The code is getting a bit awkward — a couple of entangled conditions and non-self-documenting assumptions/constraints that make me feel like it ought to be refactored. But it does work now.

Responding point by point:

Note that Christian suggested may_set_owner, not maybe_set_owner, which is clearer IMO.

This is now fixed.

You could simplify if 'set_owner' in operations or 'maybe_set_owner' in operations: as if operations in ('set_owner', 'may_set_owner'):.

operations here is a list, not a string, so this wouldn't work; it would have to be something like any(x in operations for x in ('set_owner', 'may_set_owner')) or something with sets; needless to say neither option is a simplification. :-)

For default_owner, you could write default_owner = req.authname if operation == 'set_owner' else current_owner.

This is now done (but without current_owner per the next point)

I'm not sure you can use current_owner, as it will be set to "(none)" if the ticket has no owner (it's used for printing the message). You should probably use ticket['owner'] instead.

I've changed this to ticket._old.get('owner', ticket['owner'] or None) — I'm not sure what circumstances would make that ticket._oldpart matter, but this is the way the rest of the code in default_workflow.py looks up these values.

I also included a few special-case checks in subsequent lines of code for handling the None case differently (e.g. to print "(none)" in the dropdown)

What we do in other parts of the code (e.g. when rendering a component drop-down) is to check if the current value is in the list, and if not, to add an element to the list with the current value. This makes sure it's always possible to leave the value unchanged.

Ah — I hadn't considered the case where the current owner is no longer a member of TICKET_MODIFY (for restrict_owner=true) or no longer a member of the operation.set_owner = alice, bob, carol list. I've fixed the patch to use this approach. So now when using may_set_owner it's always possible to retain the current owner, even if the current owner is None, and even if the current owner is no longer a valid option for new ticket assignments.

The operation doesn't actually do anything. :-) I neglected to make any changes to the get_ticket_changes method, so the operation is completely ignored.

This is now fixed.

[arkinform@…]

Please, patch current trunk, it's very usefull. In my project I did the same thing by patching trac sources.

[Ryan J Ollos]

The patch seems to be well-received and I can't find many issues with it so far. I've staged the changes with a small refactoring in log:rjollos.git:t10018.

There is one issue. If the user is anonymous and [ticket] restrict_owner = true, then (none) will be appended to the list rather than anonymous. This seems to be because get_users_with_permission returns None when anonymous has the permission. and None gets unconditionally replaced with (none).

It would be nice to add some tests. Writing unit tests for render_ticket_action_control would be a little bit clunky, and functional tests might be better anyway for this case. We don't seem to have many functional tests for the workflow yet. Ethan, would you be willing to add a few tests?

[ethan.jucovy@…]

Replying to rjollos:

It would be nice to add some tests. Writing unit tests for render_ticket_action_control would be a little bit clunky, and functional tests might be better anyway for this case. We don't seem to have many functional tests for the workflow yet. Ethan, would you be willing to add a few tests?

Sure, I'd be happy to. I forked your t10018 branch and started adding some functional tests here: ​https://github.com/ejucovy/trac/commits/t10018

So far I've added (somewhat clunky) tests for:

• the negative case: using set_owner, and asserting that the requesting user will be the default reassignee
• using may_set_owner with ticket.restrict_owner=false, and asserting that the ticket's previous owner will be the default reassignee in the text input
• using may_set_owner with ticket.restrict_owner=true, and asserting that the ticket's previous owner will be the default reassignee in the select dropdown, even if the previous owner is not found in env.get_known_users

I need to also add tests for:

• using may_set_owner on a ticket which has no previous owner, with restrict_owner=false and with restrict_owner=true
• using may_set_owner when the requesting user is anonymous (the issue you found in comment:14)

I'll try to finish up those missing tests over the next few days. There might be some other cases I'm missing too.

I wonder if it would be better to put them in a separate file (ticket/tests/default_workflow/functional.py?) — and I also wasn't sure whether I should call them regression tests, and name them after this ticket, or take some other approach to the naming. So let me know if you think I should reorganize them.

[Ryan J Ollos]

Replying to ethan.jucovy@…:

I wonder if it would be better to put them in a separate file (ticket/tests/default_workflow/functional.py?) —

I've considered breaking up functional.py into multiple files and putting them in a sub-directory. The file is getting pretty big, and the tests take a while to execute. I was thinking of: functional/web_ui.py, functional/roadmap.py … so functional/default_workflow.py might make sense, but maybe there are ideas from others on this.

and I also wasn't sure whether I should call them regression tests, and name them after this ticket, or take some other approach to the naming. So let me know if you think I should reorganize them.

The challenge I've run into with the naming pattern RegressionTestTicketXXXX, which is exacerbated by having so many tests in trac/ticket/tests/functional.py, is that it can be very difficult to determine if coverage exists for a particular feature. There is just way too much code to weed through. Therefore, my feeling is that if you are making a nice set of tests to get fundamental coverage for a module, it would be better to prefix all the test names with TestDefaultWorkflow, and add a link to the ticket in the comment.

[ethan.jucovy@…]

I committed a few more tests to ​my branch. I think the test suite now covers all the cases that have been discussed in these comments. I also set up a sub-directory structure for the functional tests, renaming ticket/tests/functional.py to ticket/tests/functional/main.py and moving my new tests into a new ticket/tests/functional/default_workflow.py as you suggested.

I haven't been able to reproduce the issue you described in comment:14, either in test code or in a browser:

There is one issue. If the user is anonymous and [ticket] restrict_owner = true, then (none) will be appended to the list rather than anonymous. This seems to be because get_users_with_permission returns None when anonymous has the permission. and None gets unconditionally replaced with (none).

Can you describe the reproduction steps and/or the resulting unexpected behavior in a bit more detail?

[Ryan J Ollos]

Replying to ethan.jucovy@…:

Can you describe the reproduction steps and/or the resulting unexpected behavior in a bit more detail?

I can't seem to reproduce now. Previously I think I was seeing perm.get_users_with_permission('TICKET_MODIFY') return [None, 'user1', 'user2'], but now I'm not seeing None in the list.

I'll pull down your latest changes and do some additional testing soon, and then commit if there are no issues.

[Ryan J Ollos]

Looks good. I've only found minor issues so far, and I'll post my changes later today. I've been considering whether we should move functional.py to functional/main.py at this time. It will make merging from 1.0-stable more difficult due to Subversion's subpar merging capability. I guess it probably won't be too burdensome, but if other feel differently, please let me know.

[Ryan J Ollos]

Latest changes can be found in log:rjollos.git:t10018.2:

• Hint for future: use FunctionalTestEnvironment.grant_perm and FunctionalTestEnvironment.revoke_perm and it won't be necessary to call FunctionalTestEnvironment.restart. See [11764] and #11028. After these changes execution time came down from 30+ seconds to 13 seconds. See also #11443.
• I organized some test cases into a class. The pattern is different from what is used for functional tests in the codebase, but I think it may be an improvement since we can eliminate redundant code and group related test cases.

Since there were a lot of changes here, I need to give them another review, and I'll wait a few days before committing.

[Ryan J Ollos]

Following some additional refactoring, committed to trunk in [12456:12457]. Documentation updated.

[ethan.jucovy@…]

Whoops - this broke an existing feature:

action.operations = set_owner
action.set_owner = alice, bill

In the past, this would result in a dropdown with only alice and bill, regardless of the ticket's current owner and the requesting user. (Likewise action.set_owner = bill would just render text instead of providing a dropdown at all.)

After this change, the requesting user is now present in that list always, and action.set_owner = bill will now result in a two-item dropdown when viewed by all users besides bill himself.

This can be fixed like so:

trac/ticket/default_workflow.py

@@ -265 +265 @@
                 owners.sort()
             else:
                 owners = None
+
             if owners is not None and default_owner not in owners:
-                owners.insert(0, default_owner)
+                if 'set_owner' not in this_action or 'set_owner' not in operations:
+                    owners.insert(0, default_owner)
 
             id = 'action_%s_reassign_owner' % action
             selected_owner = req.args.get(id, default_owner)

I'll work on adding another test or two to cover this behavior.

[Ryan J Ollos]

Thanks for taking care of the additional test cases. I did not manually test this specific behavior before committing.

[ethan.jucovy@…]

Proposed patch and additional tests are in attachment:t10018-set_owner-tests.diff​

Just want to double check one thing though. I think the current behavior is correct for the following configuration:

action.operations = may_set_owner
action.set_owner = bill

In this case, the current ticket owner should be (and is) injected into the list so that the operation still doesn't change the owner by default. I added tests for this behavior also, and didn't change it; I only changed it for the case when action.operations = set_owner.

[Ryan J Ollos]

Replying to ethan.jucovy@…:

Just want to double check one thing though. I think the current behavior is correct for the following configuration:

action.operations = may_set_owner
action.set_owner = bill

In this case, the current ticket owner should be (and is) injected into the list so that the operation still doesn't change the owner by default. I added tests for this behavior also, and didn't change it; I only changed it for the case when action.operations = set_owner.

Yeah, that seems correct to me. I've tried to refactor the code to make the behavior more clear. The tests currently pass. I'll do some additional manual testing and code review today.

trac/ticket/default_workflow.py

@@ -259 +259 @@
             if 'set_owner' in this_action:
                 owners = [x.strip() for x in
                           this_action['set_owner'].split(',')]
+                if 'may_set_owner' in operations and \
+                        default_owner not in owners:
+                    owners.insert(0, default_owner)
             elif self.config.getbool('ticket', 'restrict_owner'):
                 perm = PermissionSystem(self.env)
                 owners = perm.get_users_with_permission('TICKET_MODIFY')
                 owners.sort()
+                if default_owner not in owners:
+                    owners.insert(0, default_owner)
             else:
                 owners = None
 
-            if owners is not None and default_owner not in owners:
-                if 'set_owner' not in this_action or 'set_owner' not in operati
-                    owners.insert(0, default_owner)
-
             id = 'action_%s_reassign_owner' % action
             selected_owner = req.args.get(id, default_owner)

[Ryan J Ollos]

Replying to ethan.jucovy@…:

Proposed patch and additional tests are in attachment:t10018-set_owner-tests.diff​

Just want to double check one thing though. I think the current behavior is correct for the following configuration:

action.operations = may_set_owner
action.set_owner = bill

In this case, the current ticket owner should be (and is) injected into the list so that the operation still doesn't change the owner by default. I added tests for this behavior also, and didn't change it; I only changed it for the case when action.operations = set_owner.

I think there is a problem with the logic in the patch, which should cause a test case to fail, but the problem is being masked. SetOwner.test_restrict_owner_not_known_user​ should fail if anonymous is injected into the list, however it is testing for incorrect markup. The following change should be made:

-'<option value="anonymous" selected="selected"> anonymous</option>'
+'<option selected="selected" value="anonymous"> anonymous</option>'

With that change on the trunk, the test case does fail.

======================================================================
FAIL: test_restrict_owner_not_known_user (__main__.SetOwnerOperation)
When using the workflow operation `set_owner` with
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t10018/teo-rjollos.git/trac/ticket/tests/functional/default_workflow.py", line 65, in test_restrict_owner_not_known_user
    tc.notfind('<option selected="selected" value="anonymous">'
  File "trac/tests/functional/better_twill.py", line 234, in better_notfind
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ('match to \'<option selected="selected" value="anonymous">anonymous</option>\'', '/home/user/Workspace/t10018/teo-rjollos.git/testenv/trac/log/SetOwnerOperation.test_restrict_owner_not_known_user.html')

----------------------------------------------------------------------
Ran 7 tests in 12.114s

FAILED (failures=1)

It is way too easy to test for the incorrect markup with notfind. I'm going to propose a way to deal with this in forthcoming changes.

The test case still fails after applying t10018-set_owner-tests.diff​. From your comment, it sounds we want to inject default_owner only when the allowed owners are restricted and the operation is may_set_owner.

trac/ticket/default_workflow.py

@@ -265 +265 @@
                 owners.sort()
             else:
                 owners = None
-            if owners is not None and default_owner not in owners:
+            if owners is not None and default_owner not in owners \
+                    and 'may_set_owner' in operations:
                 owners.insert(0, default_owner)
 
             id = 'action_%s_reassign_owner' % action

Here's another issue: When the list of owners is restricted, the operation is may_set_owner and ticket._old['owner'] is an empty string, the empty string would be appended to the list rather than (none) when previewing the ticket.

Note: #11474 proposes to fix some issues in render_ticket_action_control that were noticed while working on this ticket.

[Ryan J Ollos]

trunk/trac/ticket/tests/functional/main.py​ has the svn:mergeinfo property set since [12457]. I'm not sure if this is helpful or just a nuisance. Typically we only have svn:mergeinfo set on the project root directory, but maybe it is somehow useful for moved files? Or maybe it's just an artifact of my "repair move" in [12457]?

[trancesilken@…]

How about set_owner defaulting to the assigned user instead of forcing it to default to the logged in user? And, if no user was assigned, it would fall back to NIL.

In addition, set_owner_to_self_opt option would be introduced, that will render a radio button "assign to me". And the list rendered by set_owner would then no longer include the logged in user.

That way, I presume, the may_reassign_to action, which I consider to be redundant, might be removed.

Of course this will change existing behaviour a bit, but who cares.

E.g.

Work = assigned → Work Work.operations = set_owner, set_owner_to_self_opt

[trancesilken@…]

Replying to trancesilken@…:

How about set_owner defaulting to the assigned user instead of forcing it to default to the logged in user? And, if no user was assigned, it would fall back to NIL.

Alternatively it would fall back to the logged in user if no user was assigned. Forget the BS about set_owner_to_self_opt.

And, if a user was already assigned, it will simply keep that assignment as the default and allow the user to edit it as required.

[Jun Omae]

Replying to rjollos:

trunk/trac/ticket/tests/functional/main.py​ has the svn:mergeinfo property set since [12457]. I'm not sure if this is helpful or just a nuisance. Typically we only have svn:mergeinfo set on the project root directory, but maybe it is somehow useful for moved files? Or maybe it's just an artifact of my "repair move" in [12457]?

Sorry for long delay. I just noticed it while working #10961.

I don't know useful cases for such moved files. We use the property for only root directory. I think we should remove from trunk/trac/ticket/tests/functional/main.py.

$ svn propget -vR svn:mergeinfo trunk branches/{1.0,0.12}-stable | grep Prop
Properties on 'trunk/trac/ticket/tests/functional/main.py':
Properties on 'trunk':
Properties on 'branches/1.0-stable':
Properties on 'branches/0.12-stable':

[Ryan J Ollos]

Thanks, svn:mergeinfo property removed in [12816].

[Ryan J Ollos]

I'll return to finish this ticket after the changes in #11474 are committed.

[Ryan J Ollos]

Fixed issue with translatable string in [13031].

[Ryan J Ollos]

Oddly the new extraction in [13036] didn't append trac/ticket/default_workflow.py:291 to trunk/trac/locale/messages.pot@13036:2443#L2441​, as would be expected with the addition of _("none"). When I moved _("none") to a variable on it's own line the instance was extracted.

This shouldn't be a problem unless the _("none") at line 235 is ever removed.

[Ryan J Ollos]

Replying to rjollos:

Note: #11474 proposes to fix some issues in render_ticket_action_control that were noticed while working on this ticket.

#11689 and #11699 also report issues found while working on this ticket.

[Ryan J Ollos]

Replying to rjollos:

I can't seem to reproduce now. Previously I think I was seeing perm.get_users_with_permission('TICKET_MODIFY') return [None, 'user1', 'user2'], but now I'm not seeing None in the list.

Sorry to delay the release, but I'm still finding issues with this ticket. I've been able to reproduce this issue now.

On the current trunk, with [ticket] restrict_owner = True and the following workflow, create a ticket with no owner.

mayassign = new,reopened -> assigned
mayassign.operations = may_set_owner
assign = new,reopened -> assigned
assign.operations = set_owner

There is a (none) entry in the assign to select that shouldn't be there.

None is inserted into the owners list when rendering the may_set_owner action control, and it is still present in the list when rendering the set_owner action control. Another way to work around the issue is to force the set_owner to render first using the default attribute of the action in the workflow.

The method get_users_with_permission returns an entry from the permission_cache, so I guess we need to copy the list if we are going to mutate it: trunk/trac/ticket/default_workflow.py@13045:261,266#L256​.

The issue is fixed by:

trac/ticket/default_workflow.py

@@ -258 +258 @@
                           this_action['set_owner'].split(',')]
             elif self.config.getbool('ticket', 'restrict_owner'):
                 perm = PermissionSystem(self.env)
-                owners = perm.get_users_with_permission('TICKET_MODIFY')
+                owners = list(perm.get_users_with_permission('TICKET_MODIFY'))
                 owners.sort()
             else:
                 owners = None

Does that look right, or is there a better way that I'm overlooking?

[Peter Suter]

Replying to rjollos:

The method get_users_with_permission returns an entry from the permission_cache, so I guess we need to copy the list if we are going to mutate it:

…

Does that look right, or is there a better way that I'm overlooking?

Your analysis looks correct to me. Good catch.

Seems like an easy mistake to make. (​another example?) Should get_users_with_permission()​ maybe return a copy of the cached list?

Instead of owners = list(...) and owners.sort() we could use owners = sorted(...).

[Ryan J Ollos]

Replying to psuter:

Seems like an easy mistake to make. (​another example?) Should get_users_with_permission()​ maybe return a copy of the cached list?

Yeah, it would be good to help plugin developers (and Trac developers) avoid the mistake.

Instead of owners = list(...) and owners.sort() we could use owners = sorted(...).

That looks good. Thanks!

browser:/trunk/trac/perm.py@12930:169-170,196#L168​ … did this ever return a dictionary?: [1860#file8].

[Ryan J Ollos]

Refactoring and regression fixes committed to trunk in [13056]. In milestone:1.1.3 I'll revisit some minor issues discovered while working on this ticket.