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.