Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11933 closed defect (fixed)

Previous owner is not notified when ticket is reassigned since 1.1.3

Reported by: Jun Omae Owned by: Peter Suter
Priority: normal Milestone: 1.1.4
Component: notification Version: 1.1.3
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fixed regression in notification of previous owner when ticket is reassigned.

API Changes:

Description

New notification system breaks #2311 feature. See also comment:4:ticket:11533.

There is NotificationTestCase.test_previous_owner in trac/ticket/tests/notification.py for the feature. However, all tests in the file don't use NotificationSystem and are dysfunctional for trunk.

We should use NotificationSystem in the tests.

Attachments (2)

T11533_notify_previous_owner_or_cc_2.diff (4.5 KB ) - added by Peter Suter 5 years ago.
T11933_cc_list_and_created_time.diff (3.3 KB ) - added by Peter Suter 5 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by Peter Suter, 5 years ago

Right, the patch in #11533 should fix this.

Yes, the tests still test the old TicketNotify classes. Switching them to test the new code sounds good. (Though some of them seem to fail then.)

by Peter Suter, 5 years ago

comment:2 by Peter Suter, 5 years ago

In #11934 ([13703]) all tests except test_previous_cc_list and test_previous_owner were converted to use NotificationSystem. This patch also converts (and fixes) these two tests.

(It's an updated version of the patch in #11533.)

in reply to:  2 ; comment:3 by Jun Omae, 5 years ago

Replying to psuter:

In #11934 ([13703]) all tests except test_previous_cc_list and test_previous_owner were converted to use NotificationSystem. This patch also converts (and fixes) these two tests.

Looks good to me except one. We could use Chrome.cc_list() to parse Cc field in CarbonCopySubscriber.matches(). According to the method, ; character is able to use in Cc field.

Two questions:

  1. Should we notify previous reporter if the reporter field is changed?
  2. Why do not we pass ticket's creation time to TicketChangeEvent when initiating ticket created event?
    -        event = TicketChangeEvent('created', ticket, None, ticket['reporter'])
    +        event = TicketChangeEvent('created', ticket, ticket['time'],
    +                                  ticket['reporter'])
    
Last edited 5 years ago by Jun Omae (previous) (diff)

in reply to:  3 ; comment:4 by Peter Suter, 5 years ago

Owner: set to Peter Suter
Status: newassigned

Thanks for the review.

Replying to jomae:

We could use Chrome.cc_list() to parse Cc field in CarbonCopySubscriber.matches(). According to the method, ; character is able to use in Cc field.

Sounds good.

  1. Should we notify previous reporter if the reporter field is changed?

That would be a new feature, right? I think it could make sense but mainly for "symmetry". (Changing reporter seems rare and I assume the previous reporter is usually obsolete or invalid.)

  1. Why do not we pass ticket's creation time to TicketChangeEvent when initiating ticket created event?
    -        event = TicketChangeEvent('created', ticket, None, ticket['reporter'])
    +        event = TicketChangeEvent('created', ticket, ticket['time'],
    +                                  ticket['reporter'])
    

No good reason, I just didn't need it so far. I agree we should pass it. Thanks again.

by Peter Suter, 5 years ago

comment:5 by Peter Suter, 5 years ago

The attached patch should be applied on top of the previous one.

Attachment deletion and batch retargeting after milestone deletion or completion still pass None instead of deletion / retargeting time, as I don't see an obvious way to get at the appropriate value.

in reply to:  4 comment:6 by Jun Omae, 5 years ago

  1. Should we notify previous reporter if the reporter field is changed?

That would be a new feature, right? I think it could make sense but mainly for "symmetry". (Changing reporter seems rare and I assume the previous reporter is usually obsolete or invalid.)

Okay. I agree that's rare and previous reporter is usually invalid. It is not needed to notify to previous reporter.

comment:7 by Peter Suter, 5 years ago

Resolution: fixed
Status: assignedclosed

Committed in [13746].

comment:8 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Peter Suter 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.