Batch modifications generate incorrect notifications

[f.poloni@…]

If I use the "batch modify" feature (on /query, available only to users with TICKET_BATCH_MODIFY), the resulting notification cannot be displayed properly by this plugin (error "report XXX does not exist" when accessing /notification).

[f.poloni@…]

Here is a possible fix.

@@ -15,7 +15,6 @@ from trac.util.compat import set
 
 from onsitenotifications.model import OnSiteMessage
 
-
 class OnSiteNotificationsDistributor(Component):
     """Distributes notification events as on-site messages."""
 
@@ -44,7 +43,16 @@ class OnSiteNotificationsDistributor(Component):
     # INotificationDistributor
 
     def transports(self):
-        yield 'on-site'
+        yield 'on-site' 
+
+    def get_target_id(self, target):
+        """
+        Workaround for batch ticket modifications
+        """
+        if isinstance(target, list):
+            return ', '.join(sorted(target))
+        else:
+            return get_target_id(target)
 
     def distribute(self, transport, recipients, event):
         if transport != 'on-site':
@@ -74,7 +82,7 @@ class OnSiteNotificationsDistributor(Component):
             message = formats[fmt].format(transport, fmt, event)
             for sid, authenticated in sids:
                 OnSiteMessage.add(self.env, sid, authenticated, message,
-                                  event.realm, get_target_id(event.target))
+                                  event.realm, self.get_target_id(event.target))
 
     # IRequestHandler methods
 
@@ -93,13 +101,30 @@ class OnSiteNotificationsDistributor(Component):
             return self._render_message(req, id)
         return self._render_list(req)
 
+    def get_summary(self, r):
+        self.env.log.error(r)
+        self.env.log.error(r.id)
+        self.env.log.error(type(r.id))
+        if r.realm == 'ticket' and ',' in r.id:
+            self.env.log.error('1')
+            return 'Batch modification of tickets %s' % r.id
+        else:
+            self.env.log.error('2')
+            return get_resource_description(self.env, r, format='summary')
+
+    def get_url(self, r, req):
+        if r.realm == 'ticket' and ',' in r.id:
+            return req.href('query', id=r.id)
+        else:
+            return get_resource_url(self.env, r, req.href)
+
+
     def _render_list(self, req):
         if req.method == 'POST':
             action = req.args.get('action')
             if action == 'deletenotifications':
                 OnSiteMessage.delete_by_sid(self.env, req.session.sid,
                                             req.session.authenticated)
-
         messages = OnSiteMessage.select_by_sid(self.env, req.session.sid,
                                                req.session.authenticated)
         items = []
@@ -107,9 +132,8 @@ class OnSiteNotificationsDistributor(Component):
             r = Resource(message['realm'], message['target'])
             items.append({
                 'resource': get_resource_description(self.env, r),
-                'details': get_resource_description(self.env, r,
-                                                    format='summary'),
-                'resource_href': get_resource_url(self.env, r, req.href),
+                'details': self.get_summary(r),
+                'resource_href': self.get_url(r, req),
                 'details_href': req.href.notification(message['id']),
             })
 

[anonymous]

Oops sorry, I left in a few "printf-debug" messages in a hurry. Please edit them out.

[Peter Suter]

In 16947:

OnSiteNotificationsPlugin: Handle batch modification notifications.
(see #13318)

[Peter Suter]

Thanks for the patch.

I see ​trac:changeset:15083#file2 uses a similar workaround for get_target_id with batch modification notifications:

if isinstance(event.target, (list, tuple)):
    targetid = ','.join(map(get_target_id, event.target))
else:
    targetid = get_target_id(event.target)

But I committed a different fix now, where batch modifications are broken down into individual ticket notifications instead.

Does that work for you? I'm not sure if it's a better approach, but it seems a bit simpler and fit better with the current UI listing resources.

[f.poloni@…]

Thanks for your quick action!

I can see the merits of your approach, which is really neat and simple, but personally as a user I would find clearer to keep the notifications in batch format: it is simpler for me to process "These 50 tickets have been closed" than "Ticket 131 has been closed. Ticket 132 has been closed. Ticket 133 has been closed...."

That's just a personal opinion though, and I totally understand if you prefer your choice.