Edgewall Software
Modify

Opened 18 years ago

Closed 9 years ago

Last modified 2 years ago

#2259 closed defect (fixed)

No notification email when adding an attachment to a ticket

Reported by: webkontakt@… Owned by: Ryan J Ollos
Priority: high Milestone: 1.0.3
Component: attachment Version: devel
Severity: major Keywords: notification
Cc: webkontakt@…, kazarmy@…, trac@…, mjlucas@…, stepan@…, tracedgewall@…, fredck@…, shepting@…, itamarost@…, felix.schwarz@…, greg@…, haggai.eran@…, Thijs Triemstra, ezyang@…, Steffen Hoffmann, keshav.kini@…, gasolwu@…, leho@… Branch:
Release Notes:

Notification email is sent when a ticket attachment is added or deleted and TicketAttachmentNotifier is enabled.

API Changes:
Internal Changes:

Description

If you just add an attachment to a ticket and change nothing else, no notification emails are sent, even if the CC-Field is filled.

Btw., it's very hard to change anything else (e.g. adding a comment) when adding an attachment, so noone does. Therefore notification is necessary there.

Attachments (8)

trac_ticket_r2659.patch (1.2 KB ) - added by trac@… 18 years ago.
patch for notification only. against r2659
trac_ticket_r2659.2.patch (1.1 KB ) - added by trac@… 18 years ago.
This patch actually has contents in the email. Sorry about that…
trac_ticket_r2659.3.patch (2.6 KB ) - added by trac@… 18 years ago.
full functionality requested. adds remark on timeline
trac_ticket_r6106.patch (1.4 KB ) - added by sid 16 years ago.
Update previous submissions to be in line with changes in trunk
attachment_0.13dev.patch (15.2 KB ) - added by Emmanuel Blot 13 years ago.
New patch proposal for current trunk (0.13dev)
attachment_0.13dev.2.patch (15.0 KB ) - added by Emmanuel Blot 13 years ago.
Fix up some issues with the previous patch, all unit/functional tests now pass Ok.
add-attachment-description.diff (1.9 KB ) - added by Christian Boos 13 years ago.
Also send attachment's description in the body of the notificatio n mail. Applies on top of attachment_0.13dev.2.patch.
attachment_1.0.1dev.patch (15.9 KB ) - added by Emmanuel Blot 11 years ago.
New patch for latest development version (1.0.1dev), including cboos fixes.

Download all attachments as: .zip

Change History (73)

comment:1 by anonymous, 18 years ago

Cc: webkontakt@… added

comment:2 by Markus Tacker <m@…>, 18 years ago

It should also appear on the timeline.

comment:3 by anonymous, 18 years ago

Cc: webkontakt@… added; webkontakt@… removed
Version: 0.8.40.9b2

comment:4 by Christian Boos, 18 years ago

Related to #1498.

comment:5 by anonymous, 18 years ago

Version: 0.9b20.9

by trac@…, 18 years ago

Attachment: trac_ticket_r2659.patch added

patch for notification only. against r2659

comment:6 by trac@…, 18 years ago

Cc: webkontakt@… kazarmy@… trac@… added; webkontakt@… removed
Version: 0.9devel

Added attachment for a patch against r2659 that takes care of only the email notification portion of this ticket. Currently does nothing for adding the attachment detail to the timeline.

by trac@…, 18 years ago

Attachment: trac_ticket_r2659.2.patch added

This patch actually has contents in the email. Sorry about that…

by trac@…, 18 years ago

Attachment: trac_ticket_r2659.3.patch added

full functionality requested. adds remark on timeline

comment:7 by trac@…, 18 years ago

Resolution: fixed
Status: newclosed

The new patch contains both the email notification for attachments and will add the line "attachment changed" to the timeline when the ticket_show_details configuration option is set (see TracIni).

comment:8 by Matthew Good, 18 years ago

Resolution: fixed
Status: closedreopened

Please don't mark a ticket as fixed unless the patch has been committed.

comment:9 by Christian Boos, 18 years ago

Milestone: 1.0
Owner: changed from Jonas Borgström to Christian Boos
Status: reopenednew

I have now a few days off the net and, among other things, I'll test the patch (the timeline part, Emmanuel, can you take care of the evaluating the notification part?)

comment:10 by Emmanuel Blot, 18 years ago

Sure, I'll do that by the end of this week

comment:11 by Emmanuel Blot, 18 years ago

Keywords: notification email added

comment:12 by Emmanuel Blot, 18 years ago

#2897 has been marked as a (partial) duplicate of this ticket.

comment:13 by anonymous, 18 years ago

Cc: mjlucas@… added

comment:14 by mjlucas@…, 18 years ago

What is the status of the defect? I have applied the patch.3 to my version of 0.9.3 for testing. The notification section (attachment.py) works as specified and seems to send the correct information to the user (Will be doing testing over the next few days)

However the modifications to web_ui.py seem to cause all sorts of problems. I don't know if it is because of changes landed after this patch was written or what but the timeline gets screwed up with tickets appearing as done on 1/1/1970 (ie. no timestamp) and the firelds appear to be out of place. I haven't looked at what exactly is returned by the SQL statement however if there is a newer patch could someone please post it online.

comment:15 by Christian Boos, 18 years ago

Milestone: 1.00.11

This ticket will require some general changes on the way the attachment changes interfere with the main resource changes.

See also #1278, which is about a related issue, but for wiki pages.

comment:16 by Alec Thomas, 18 years ago

r3400 adds attachment timeline events for Wiki pages and tickets.

comment:17 by sid, 17 years ago

#2725 is similar (Ticket changetime is not updated with an attachment) and has a patch.

comment:18 by Stefan Reinauer <stepan@…>, 17 years ago

Cc: stepan@… added
Keywords: attachment added
Severity: minorblocker

Hm, bug juggling. I came from #4075 because trac is not attaching ticket attachments to email notifications. This is a critical issue and people are urging me to drop trac for LinuxBIOS again (www.linuxbios.org) because of this.

Unfortunately above patches are a year old and don't apply to our version of trac (0.10) nor do they work if applied manually (server error).

This bug has been there since 0.8.4. Will it ever be fixed?

comment:19 by Emmanuel Blot, 17 years ago

Severity: blockermajor

Come on, be serious, this bug does not prevent from using Trac.

If you want it to be fixed quicker, patches are welcome. As you can see, it is scheduled for 0.11.

comment:20 by stepan@…, 17 years ago

I am pretty serious about this one. The common sense seems to be "we dont use browsers because email clients are more concise". While shaking your head might be an appropriate reaction, I still want to keep using trac for this project.

If I manage to come up with a viable solution, I will of course drop a patch to this ticket.

Anyways, thanks for the quick life sign ;-)

comment:21 by Emmanuel Blot, 17 years ago

Keywords: email removed

See also #4281.

comment:22 by Christian Boos, 17 years ago

Milestone: 0.110.12

comment:23 by anonymous, 17 years ago

Is the internal structure of Trac so inflexible that this can not be implemented easily? Someone complained in 0.10 and got put off until 0.11. Now it's 0.12. Why not set it to 2.0 now.

comment:24 by Christian Boos, 17 years ago

Ever wondered how Trac manages to go forward? Look at the timeline.

If it's 0.12, it's for a good reason, or rather 3:

  • nobody provided a patch that could accelerate the process to get this feature in
  • there is a bunch of other notification related changes that will hopefully happen in 0.12, so it's natural to group it there
  • there's already too many things scheduled for 0.11 and most likely 2/3 of the still opened 181 tickets (as of this writing) for 0.11 will go to 0.12, if not later.

comment:25 by Christian Boos, 17 years ago

Cc: tracedgewall@… added

#5721 was closed as duplicate. I've added its reporter to the CC: list.

by sid, 16 years ago

Attachment: trac_ticket_r6106.patch added

Update previous submissions to be in line with changes in trunk

comment:26 by sid, 16 years ago

I attached an untested patch that does the same thing as the previous patch (minus the timeline work). It should conform to the changes that have happened in trunk since. Any feedback from people willing to test this would be appreciated (and greatly help its chances of getting merged to trunk sooner).

comment:27 by anton, 16 years ago

It would be nice if this would work in the 0.11 version.

I set up a trac to communicate with some developers of another company for a project.

I set up mail notification too.

Actually this issue is a problem, because this trac is only for one project between our companies.

Since in both companies the peoples are working on more than one project, its better to be notified by mail than to log in every day (perhaps in other issue / bug systems for different projects) to look in the timeline what did change (even if nothing changed).

comment:28 by Christian Boos, 16 years ago

Milestone: 0.120.11

sid, thanks for the patch, I did notice it but apparently forgot to test it.

I'll take care of adding that in time for 0.11, after the context-refact merge.

comment:29 by Christian Boos, 16 years ago

Component: ticket systemattachment
Keywords: attachment removed
Milestone: 0.110.12
Priority: normalhigh

Sorry, changed my mind. This has to be redone in a more generic way in 0.12, as it's somewhat a bit unclean to hard-code ticket notifications that way, now that the attachment module is really fully generic.

For those desperately in the need for the feature in the meantime, there's the patch…

comment:30 by niels.reedijk@…, 16 years ago

Is this supposed to work? The patch does not apply cleanly to the current trunk, and if I manually merge it, all modules importing Attachment fail. Could I be doing something wrong, or is it the patch?

comment:31 by fredck@…, 16 years ago

Cc: fredck@… added

comment:32 by anonymous, 16 years ago

Milestone: 0.130.12

comment:33 by shepting@…, 15 years ago

Cc: shepting@… added

Has anyone been able to provide a working patch for 0.11 yet? My company would be interested in using this in a production setting, but just as niels.reedijk noted there are unrecoverable errors when applying this patch.

comment:34 by Christian Boos, 15 years ago

There's some discussion on trac-dev about a possible re-implementation of notifications (googlegroups:trac-dev:251bb1e6129dd095). In the meantime you might try the TracHacks:AnnouncerPlugin.

OTOH, adapting the patch to work with 0.11 or trunk shouldn't be hard, I'll give it a try later today.

comment:35 by Remy Blank, 14 years ago

Milestone: 0.12next-major-0.1X

It seems that the notification revamp will not happen in 0.12.

comment:36 by peshing@…, 14 years ago

hello,

is there, a working patch for trac 0.11. I tryed to make it wotk but I cannot import the Ticket model in attachment.py and that is why the patch is not working…, but I am new to python aniway so please help

comment:37 by Itamar Ostricher, 14 years ago

Cc: itamarost@… added

comment:38 by Felix Schwarz, 14 years ago

Cc: felix.schwarz@… added

comment:39 by greg@…, 13 years ago

Cc: greg@… added

comment:40 by haggai.eran@…, 13 years ago

Cc: haggai.eran@… added

comment:41 by quintin@…, 13 years ago

How's about someone just takes one of the patches and apply it to trunk? Many people are waiting for this change to happen.

Make the timeline integration a second ticket? It's a small job to apply one of the patches to trunk. Those who know how to do it themselves do so, but many people just download and use as-is.

comment:42 by Thijs Triemstra, 13 years ago

Cc: Thijs Triemstra added

comment:43 by ezyang@…, 13 years ago

Cc: ezyang@… added

by Emmanuel Blot, 13 years ago

Attachment: attachment_0.13dev.patch added

New patch proposal for current trunk (0.13dev)

comment:44 by Steffen Hoffmann, 13 years ago

Cc: Steffen Hoffmann added

by Emmanuel Blot, 13 years ago

Attachment: attachment_0.13dev.2.patch added

Fix up some issues with the previous patch, all unit/functional tests now pass Ok.

comment:45 by Christian Boos, 13 years ago

Just tested it and it works fine.

Nit-picking, currently the comment in the mail is simply:

Add attachment "<filename>"

I'd prefer something like:

Attachment "<filename>" added.

(the attachment description)

by Christian Boos, 13 years ago

Also send attachment's description in the body of the notificatio n mail. Applies on top of attachment_0.13dev.2.patch.

comment:46 by keshav.kini@…, 12 years ago

Cc: keshav.kini@… added

comment:47 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Cc: ryan.j.ollos@… added

by Emmanuel Blot, 11 years ago

Attachment: attachment_1.0.1dev.patch added

New patch for latest development version (1.0.1dev), including cboos fixes.

comment:48 by Gasol Wu <gasolwu@…>, 11 years ago

Cc: gasolwu@… added

comment:49 by Gasol Wu <gasolwu@…>, 11 years ago

I have written small plugin called AttachmentNotifyPlugin, based on patches of this thread. For someone waiting for 7 years.

comment:50 by Ryan J Ollos, 10 years ago

Cc: ryan.j.ollos@… removed
Milestone: next-major-releases1.0.3
Owner: changed from Christian Boos to Ryan J Ollos
Status: newassigned

Tentatively scheduling for 1.0.3, or possibly 1.1.3.

comment:51 by lkraav <leho@…>, 10 years ago

Cc: leho@… added

comment:52 by Ryan J Ollos, 9 years ago

attachment:attachment_1.0.1dev.patch was rebased on 1.0-stable in log:rjollos.git:t2259-notification-email-when-adding-attachment-to-ticket. The attachment notification is functioning correctly in the limited amount of testing that I've done. I plan to further review the patch.

Shouldn't these class attributes be instance attributes instead?: tags/trac-1.0.1/trac/ticket/notification.py@:136-138#L132. They are modified within an instance method: tags/trac-1.0.1/trac/ticket/notification.py@:162-164#L132.

in reply to:  52 comment:53 by Ryan J Ollos, 9 years ago

Replying to rjollos:

Shouldn't these class attributes be instance attributes instead?: tags/trac-1.0.1/trac/ticket/notification.py@:136-138#L132. They are modified within an instance method: tags/trac-1.0.1/trac/ticket/notification.py@:162-164#L132.

Change committed to 1.0-stable in [13372] and merged to trunk in [13373].

comment:54 by Ryan J Ollos, 9 years ago

Release Notes: modified (diff)

Refactoring in [13374,13376], merged to trunk in [13375,13377].

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

comment:55 by Ryan J Ollos, 9 years ago

Some refinements:

  • Wrapped length of attachment added / attachment deleted and attachment description texts.
  • Translation deactivation only encloses necessary code.
  • Added try/except around notify_attachment.
  • notify method is mostly unmodified.

Proposed changes in log:rjollos.git:t2259-notification-email-when-adding-attachment-to-ticket.1.

comment:56 by Ryan J Ollos, 9 years ago

Release Notes: modified (diff)

in reply to:  49 comment:57 by Ryan J Ollos, 9 years ago

Replying to Gasol Wu <gasolwu@…>:

I have written small plugin called AttachmentNotifyPlugin, based on patches of this thread. For someone waiting for 7 years.

Thank you for providing an interim solution. I've added a note to the th:AttachmentNotifyPlugin to indicate that it won't be needed for Trac ≥ 1.0.3.

in reply to:  55 ; comment:58 by Jun Omae, 9 years ago

Proposed changes in log:rjollos.git:t2259-notification-email-when-adding-attachment-to-ticket.1.

Quick testing, seems like good. But I think _translation_deactivated is wrong when an exception is raised while executing yield. In the case, the statements after yield wouldn't be executed.

from __future__ import with_statement
from contextlib import contextmanager

@contextmanager
def fn():
    print 'pre'
    yield
    print 'post'

def raising():
    raise ValueError

def main():
    print 'start'
    try:
        with fn():
            raising()
    except ValueError:
        print 'caught'
    print 'end'

main()
$ python2.5 /tmp/test.py
start
pre
caught
end

in reply to:  58 comment:59 by Ryan J Ollos, 9 years ago

Replying to jomae:

Proposed changes in log:rjollos.git:t2259-notification-email-when-adding-attachment-to-ticket.1.

Quick testing, seems like good. But I think _translation_deactivated is wrong when an exception is raised while executing yield. In the case, the statements after yield wouldn't be executed.

Thanks, I'll modify the context manager:

from __future__ import with_statement
from contextlib import contextmanager

@contextmanager
def fn():
    print 'pre'
    try:
        yield
    finally:
        print 'post'

def raising():
    raise ValueError

def main():
    print 'start'
    try:
        with fn():
            raising()
    except ValueError:
        print 'caught'
    print 'end'

main()
start
pre
post
caught
end

in reply to:  58 comment:60 by Ryan J Ollos, 9 years ago

Replying to jomae:

Quick testing, seems like good. But I think _translation_deactivated is wrong when an exception is raised while executing yield. In the case, the statements after yield wouldn't be executed.

I modified the context manager to handle that scenario in [8ebece33/rjollos.git].

Additional refactoring in [13378], merged in [13379].

comment:61 by Jun Omae, 9 years ago

Unit tests failing with Python 2.7 and TRAC_TEST_DB_URI=sqlite:test.db. All tests pass with Python 2.5 and 2.6.

self.env.reset_db() in tearDown() raises an OperationalError.

...
======================================================================
ERROR: test_ticket_notify_attachment_enabled_attachment_removed (trac.ticket.tests.notification.AttachmentNotificationTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jun66j5/src/tracdev/git/trac/ticket/tests/notification.py", line 1277, in tearDown
    self.env.reset_db()
  File "/home/jun66j5/src/tracdev/git/trac/test.py", line 349, in reset_db
    tables = getattr(m, reset_fn)(self, db_prop)
  File "/home/jun66j5/src/tracdev/git/trac/test.py", line 184, in reset_sqlite_db
    db("DELETE FROM %s" % table)
  File "/home/jun66j5/src/tracdev/git/trac/db/util.py", line 128, in execute
    cursor.execute(query, params if params is not None else [])
  File "/home/jun66j5/src/tracdev/git/trac/db/util.py", line 63, in execute
    r = self.cursor.execute(sql)
  File "/home/jun66j5/src/tracdev/git/trac/db/sqlite_backend.py", line 80, in execute
    result = PyFormatCursor.execute(self, *args)
  File "/home/jun66j5/src/tracdev/git/trac/db/sqlite_backend.py", line 58, in execute
    args or [])
  File "/home/jun66j5/src/tracdev/git/trac/db/sqlite_backend.py", line 50, in _rollback_on_error
    return function(self, *args, **kwargs)
OperationalError: disk I/O error

----------------------------------------------------------------------
Ran 1633 tests in 29.409s

FAILED (errors=4)
make: *** [unit-test] Error 1

I think we should call rmtree after reset_db because rmtree removes the SQLite database file.

  • trac/ticket/tests/notification.py

    diff --git a/trac/ticket/tests/notification.py b/trac/ticket/tests/notification.py
    index 9476cba..e101214 100644
    a b class AttachmentNotificationTestCase(unittest.TestCase):  
    12731273    def tearDown(self):
    12741274        """Signal the notification test suite that a test is over"""
    12751275        notifysuite.tear_down()
    1276         shutil.rmtree(self.env.path)
    12771276        self.env.reset_db()
     1277        shutil.rmtree(self.env.path)
    12781278
    12791279    def test_ticket_notify_attachment_enabled_attachment_added(self):
    12801280        self.attachment.insert('foo.txt', StringIO(''), 1)
Version 0, edited 9 years ago by Jun Omae (next)

comment:62 by Ryan J Ollos, 9 years ago

Resolution: fixed
Status: assignedclosed

Fixed issue in comment:60 and also fixed change author email not being obfuscated. Committed to 1.0-stable in [13380:13382], merged to trunk in [13383:13385].

comment:63 by Ryan J Ollos, 9 years ago

I was thinking about whether we really need the option [notification] ticket_notify_attachment. The TicketAttachmentNotifier can already be disabled by disabling the component: trac.ticket.notification.ticketattachmentnotifier = disabled. It is always nice to have one fewer option and we could just document how to disable the component on the TracNotification page. Do that seem straightforward enough, or does anyone see a reason to keep the [notification] option?

  • trac/ticket/notification.py

    diff --git a/trac/ticket/notification.py b/trac/ticket/notification.py
    index cace8ee..d34e3ee 100644
    a b class TicketAttachmentNotifier(Component):  
    502502
    503503    implements(IAttachmentChangeListener)
    504504
    505     ticket_notify_attachment = BoolOption('notification',
    506                                           'ticket_notify_attachment', True,
    507         """Always send notifications on ticket attachment events.""")
    508 
    509505    # IAttachmentChangeListener methods
    510506
    511507    def attachment_added(self, attachment):
    512         if self.ticket_notify_attachment:
    513             self._notify_attachment(attachment, True)
     508        self._notify_attachment(attachment, True)
    514509
    515510    def attachment_deleted(self, attachment):
    516         if self.ticket_notify_attachment:
    517             self._notify_attachment(attachment, False)
     511        self._notify_attachment(attachment, False)
    518512
    519513    def attachment_reparented(self, attachment, old_parent_realm,
    520514                              old_parent_id):
Last edited 9 years ago by Ryan J Ollos (previous) (diff)

in reply to:  63 comment:64 by Peter Suter, 9 years ago

Replying to rjollos:

I was thinking about whether we really need the option [notification] ticket_notify_attachment. The TicketAttachmentNotifier can already be disabled by disabling the component: trac.ticket.notification.ticketattachmentnotifier = disabled. It is always nice to have one fewer option and we could just document how to disable the component on the TracNotification page. Do that seem straightforward enough, or does anyone see a reason to keep the [notification] option?

I agree, disabling the component seems just as good to me.

comment:65 by Ryan J Ollos, 9 years ago

Release Notes: modified (diff)

Thanks for the review. Edited the documentation in TracNotification@85. Committed to 1.0-stable in [13434], merged to trunk in [13435].

Modify Ticket

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