Edgewall Software
Modify

Opened 13 years ago

Closed 10 years ago

Last modified 10 years ago

#10018 closed defect (fixed)

no apparent way to make set_owner default to existing owner

Reported by: matkinson@… Owned by: ethan.jucovy@…
Priority: normal Milestone: 1.1.2
Component: ticket system Version: 0.12-stable
Severity: normal Keywords: workflow, owner
Cc: marc31boss@…, ethan.jucovy@…, arkinform@… Branch:
Release Notes:

Added may_set_owner workflow operation which is like set_owner but the value defaults to the existing owner.

API Changes:
Internal Changes:

Description

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.

Attachments (5)

maybe_set_owner.patch (1.6 KB ) - added by ethan.jucovy@… 11 years ago.
Adds new workflow operation maybe_set_owner
may_set_owner.working.patch (2.7 KB ) - added by ethan.jucovy@… 11 years ago.
improved patch
t10018-set_owner-tests.diff (8.3 KB ) - added by ethan.jucovy@… 10 years ago.
before_preview.png (36.1 KB ) - added by Ryan J Ollos 10 years ago.
after_preview.png (34.0 KB ) - added by Ryan J Ollos 10 years ago.

Download all attachments as: .zip

Change History (49)

comment:1 by Christian Boos, 13 years ago

Component: generalticket system
Milestone: unscheduled

So eventually have an action "may_set_owner"? PatchWelcome.

comment:2 by Marc Plano-Lesay <marc31boss@…>, 12 years ago

Cc: marc31boss@… added

comment:3 by anonymous, 11 years ago

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

comment:4 by anonymous, 11 years ago

oops I mean .12.2

comment:5 by commerce@…, 11 years ago

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.

comment:6 by commerce@…, 11 years ago

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.

by ethan.jucovy@…, 11 years ago

Attachment: maybe_set_owner.patch added

Adds new workflow operation maybe_set_owner

comment:7 by ethan.jucovy@…, 11 years ago

Cc: ethan.jucovy@… added

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

comment:8 by Remy Blank, 11 years ago

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.

comment:9 by ethan.jucovy@…, 11 years ago

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.

in reply to:  9 comment:10 by Remy Blank, 11 years ago

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.

by ethan.jucovy@…, 11 years ago

Attachment: may_set_owner.working.patch added

improved patch

comment:11 by ethan.jucovy@…, 11 years ago

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.

comment:12 by arkinform@…, 10 years ago

Cc: arkinform@… added

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

comment:13 by Ryan J Ollos, 10 years ago

Cc: Ryan J Ollos added

comment:14 by Ryan J Ollos, 10 years ago

Milestone: unscheduled1.1.3
Owner: set to Ryan J Ollos
Status: newassigned

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?

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  14 ; comment:15 by ethan.jucovy@…, 10 years ago

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.

in reply to:  15 comment:16 by Ryan J Ollos, 10 years ago

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/ticket.py, functional/milestone.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.

Version 1, edited 10 years ago by Ryan J Ollos (previous) (next) (diff)

comment:17 by ethan.jucovy@…, 10 years ago

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?

in reply to:  17 ; comment:18 by Ryan J Ollos, 10 years ago

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.

comment:19 by Ryan J Ollos, 10 years ago

Milestone: 1.1.31.1.2

comment:20 by Ryan J Ollos, 10 years ago

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.

comment:21 by Ryan J Ollos, 10 years ago

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.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:22 by Ryan J Ollos, 10 years ago

Cc: Ryan J Ollos removed
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:23 by Ryan J Ollos, 10 years ago

Owner: changed from Ryan J Ollos to Ethan Jucovy <ethan.jucovy@…>

comment:24 by ethan.jucovy@…, 10 years ago

Resolution: fixed
Status: closedreopened

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

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index 8a00321..01e0055 100644
    a b Read TracWorkflow for more information (don't forget to 'wiki upgrade' as well)  
    265265                owners.sort()
    266266            else:
    267267                owners = None
     268
    268269            if owners is not None and default_owner not in owners:
    269                 owners.insert(0, default_owner)
     270                if 'set_owner' not in this_action or 'set_owner' not in operations:
     271                    owners.insert(0, default_owner)
    270272
    271273            id = 'action_%s_reassign_owner' % action
    272274            selected_owner = req.args.get(id, default_owner)

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

comment:25 by Ryan J Ollos, 10 years ago

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

by ethan.jucovy@…, 10 years ago

Attachment: t10018-set_owner-tests.diff added

comment:26 by ethan.jucovy@…, 10 years ago

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.

comment:27 by Ryan J Ollos, 10 years ago

Owner: changed from Ethan Jucovy <ethan.jucovy@…> to Ryan J Ollos
Status: reopenedassigned

in reply to:  26 comment:28 by Ryan J Ollos, 10 years ago

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

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index 01e0055..815f57e 100644
    a b Read TracWorkflow for more information (don't forget to 'w  
    259259            if 'set_owner' in this_action:
    260260                owners = [x.strip() for x in
    261261                          this_action['set_owner'].split(',')]
     262                if 'may_set_owner' in operations and \
     263                        default_owner not in owners:
     264                    owners.insert(0, default_owner)
    262265            elif self.config.getbool('ticket', 'restrict_owner'):
    263266                perm = PermissionSystem(self.env)
    264267                owners = perm.get_users_with_permission('TICKET_MODIFY')
    265268                owners.sort()
     269                if default_owner not in owners:
     270                    owners.insert(0, default_owner)
    266271            else:
    267272                owners = None
    268273
    269             if owners is not None and default_owner not in owners:
    270                 if 'set_owner' not in this_action or 'set_owner' not in operati
    271                     owners.insert(0, default_owner)
    272 
    273274            id = 'action_%s_reassign_owner' % action
    274275            selected_owner = req.args.get(id, default_owner)

in reply to:  26 ; comment:29 by Ryan J Ollos, 10 years ago

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

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index 8a00321..1eac832 100644
    a b Read TracWorkflow for more information (don't forget to 'wiki upgrade' as well)  
    265265                owners.sort()
    266266            else:
    267267                owners = None
    268             if owners is not None and default_owner not in owners:
     268            if owners is not None and default_owner not in owners \
     269                    and 'may_set_owner' in operations:
    269270                owners.insert(0, default_owner)
    270271
    271272            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.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

by Ryan J Ollos, 10 years ago

Attachment: before_preview.png added

by Ryan J Ollos, 10 years ago

Attachment: after_preview.png added

comment:30 by Ryan J Ollos, 10 years ago

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]?

comment:31 by trancesilken@…, 10 years ago

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

in reply to:  31 comment:32 by trancesilken@…, 10 years ago

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.

in reply to:  30 comment:33 by Jun Omae, 10 years ago

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':

comment:34 by Ryan J Ollos, 10 years ago

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

comment:35 by Ryan J Ollos, 10 years ago

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

comment:36 by Ryan J Ollos, 10 years ago

Release Notes: modified (diff)

comment:37 by Ryan J Ollos, 10 years ago

Fixed issue with translatable string in [13031].

comment:38 by Ryan J Ollos, 10 years ago

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.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  29 comment:39 by Ryan J Ollos, 10 years ago

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.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  18 ; comment:40 by Ryan J Ollos, 10 years ago

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

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index cd91f8a..5f8fc02 100644
    a b Read TracWorkflow for more information (don't forget to 'wik  
    258258                          this_action['set_owner'].split(',')]
    259259            elif self.config.getbool('ticket', 'restrict_owner'):
    260260                perm = PermissionSystem(self.env)
    261                 owners = perm.get_users_with_permission('TICKET_MODIFY')
     261                owners = list(perm.get_users_with_permission('TICKET_MODIFY'))
    262262                owners.sort()
    263263            else:
    264264                owners = None

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

in reply to:  40 ; comment:41 by Peter Suter, 10 years ago

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

Last edited 10 years ago by Peter Suter (previous) (diff)

in reply to:  41 comment:42 by Ryan J Ollos, 10 years ago

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

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:43 by Ryan J Ollos, 10 years ago

Resolution: fixed
Status: assignedclosed

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.

comment:44 by Ryan J Ollos, 10 years ago

Owner: changed from Ryan J Ollos to ethan.jucovy@…

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain ethan.jucovy@….
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from ethan.jucovy@… to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.