Bugs and Improvement for batch modification for 'keywords' like fields

[beachroad]

Thanks for your recent changes for keyword modification but it has a bug and it doesn't satisify my requirement very well. So I wrote a short patch as below:

• Introduce 3 options for customization
  • [batchmod] fields_as_list
  • [batchmod] list_separator_regex
  • [batchmod] list_connecter_string
• Fix a bug when applying 'keyword'-type modification for multiple tickets.
• Using Trac 0.11.6, after applying batch modification, I redirect to 'default query' page. I'm not sure but Trac development team change a policy to process requests for '/query' page. I replaced 'actionURL' data by 'query_href' to redirect explicity. (Though actionURL itself didn't work by typo...)

batchmod/web_ui.py

@@ -3 +3 @@
 # Copyright (C) 2007 Dave Gynn
 
 from trac.core import *
+from trac.config import Option, ListOption
 from trac.perm import IPermissionRequestor
 from trac.ticket import TicketSystem, Ticket
 from trac.ticket.query import QueryModule
@@ -18 +19 @@
 class BatchModifyModule(Component):
     implements(IPermissionRequestor, ITemplateProvider, IRequestFilter, ITemplateStreamFilter)
 
+    fields_as_list = ListOption("batchmod", "fields_as_list", default="keywords",
+                                doc="field names modified as a value list(separated by ',')")
+    list_separator_regex = Option("batchmod", "list_separator_regex", default='[,\s]+',
+                                  doc="separator regex used for 'list' fields")
+    list_connecter_string = Option("batchmod", "list_connector_string", default=',',
+                                   doc="conneter string for 'list' fields")
+
     # IPermissionRequestor methods
     def get_permission_actions(self):
         yield 'TICKET_BATCH_MODIFY'
@@ -37 +45 @@
             req.args.get('batchmod') and self._has_permission(req):
             self.log.debug('BatchModifyModule: executing')
             self._batch_modify(req)
+            # redirect to original Query
+            # (FIMXE: more good way to fake QueryModule...)
+            req.redirect(req.args.get('query_href'))
+
         return handler
 
     def post_process_request(self, req, template, content_type):
@@ -75 +87 @@
                 if not modify_changetime:
                   original_changetime = to_timestamp(t.time_changed)
 
-                if 'keywords' in values:
-                    values['keywords'] = self._merge_keywords(t.values['keywords'], values['keywords'])
+                _values = values.copy()
+                for field in [f for f in values.keys() if f in self.fields_as_list]:
+                    _values[field] = self._merge_keywords(t.values[field], values[field])
 
-                t.populate(values)
+                t.populate(_values)
                 t.save_changes(req.authname, comment)
 
                 if not modify_changetime:
@@ -107 +120 @@
         self.log.debug('BatchModifyPlugin: existing keywords are %s', original_keywords)
         self.log.debug('BatchModifyPlugin: new keywords are %s', new_keywords)
 
-        regexp = re.compile(r'[^-\w]+')
+        regexp = re.compile(self.list_separator_regex)
 
-        new_keywords = set([k.strip() for k in regexp.split(new_keywords) if k])
-        combined_keywords = set([k.strip() for k in regexp.split(original_keywords) if k])
+        new_keywords = [k.strip() for k in regexp.split(new_keywords) if k]
+        combined_keywords = [k.strip() for k in regexp.split(original_keywords) if k]
 
         for keyword in new_keywords:
             if keyword.startswith('-'):
                 keyword = keyword[1:]
-                if keyword in combined_keywords:
+                while keyword in combined_keywords:
                     combined_keywords.remove(keyword)
             else:
-                combined_keywords.add(keyword)
+                if keyword not in combined_keywords:
+                    combined_keywords.append(keyword)
 
         self.log.debug('BatchModifyPlugin: combined keywords are %s', combined_keywords)
-        return ', '.join(combined_keywords)
+        return self.list_connecter_string.join(combined_keywords)
 
     # ITemplateStreamFilter methods
     def filter_stream(self, req, method, filename, stream, formdata):
@@ -134 +148 @@
 
     def _generate_form(self, req, data):
         batchFormData = dict(data)
-        batchFormData['actionUri']= req.session['query_href'] or req.href.query()
+        batchFormData['query_href']= req.session['query_href'] or req.href.query()
 
         ticketSystem = TicketSystem(self.env)
         fields = []

batchmod/templates/batchmod.html

@@ -5 +5 @@
      xmlns:py="http://genshi.edgewall.org/"
      xmlns:xi="http://www.w3.org/2001/XInclude">
 <body>
-<form id="batchmod-form" method="post" action="${actionURI}">
+<form id="batchmod-form" method="post">
 <fieldset id="batchmod-fieldset"><legend class="foldable">Batch Modify Properties</legend>
 <table class="properties">
      <tr py:for="row in group(fields, 2, lambda f: f.type != 'textarea')"
@@ -53 +53 @@
 </table>
 <div>
     <input type="hidden" name="selectedTickets" value=""/>
+    <input type="hidden" name="query_href" value="${query_href}"/>
     <input type="submit" id="batchmod" name="batchmod" value="Change tickets" />
     <input type="checkbox" id="bmod_modify_changetime" name="bmod_modify_changetime" value="true" checked="checked" />Modify Ticket Changetime
 </div>
@@ -105 +106 @@
 </script>
 </form>
 </body>
-</html>
- No newline at end of file
+</html>

[CuriousCurmudgeon]

What was the bug you fixed with keyword modifications for multiple tickets? I see the changes you made, but I don't know what problems you were seeing.

The rest of it looks good. You beat me to implementing the configuration options. I was going to do that myself soon, but you saved me the work. :)

[CuriousCurmudgeon]

I am unable to apply the patch for some reason. Can you try doing an update and attaching the .patch file to the ticket?

[CuriousCurmudgeon]

(In [8120]) refs #7181

• Applied patch to 0.11 and 0.12 versions. It wouldn't apply automatically for some reason so I did it manually.

[anonymous]

Replying to CuriousCurmudgeon:

What was the bug you fixed with keyword modifications for multiple tickets? I see the changes you made, but I don't know what problems you were seeing.

I guess you've already noticed... You use 'merge_keywords and populate' like:

values['keywords'] = merge_keyowrds(t.values['keywords'], values['keywords'])
t.populate(values)

But 'values' was never initialized for each tickets, so the 'merged' result was used for 'requests' for the following tickets.

For example, you have two tickets with keywords

• #1 - a,b
• #2 - c,d

and you 'batchmod' w/ 'e,-b'. After the plugin processes #1, 'values' became 'a,e' as the result of 'merge_keyword'.
And it was applied to #2 as the 'request', so #2 came 'c,d,a,e'.

So I changed to preserve the original requests.

One more changes is to keep the original order as possible. You used 'set()' and it's completely correct for in this usage, but I want to keep original order, it's just my flavor but for non-alphabet keywords, simple 'sort()' doesn't work and I didn't have good way to give some order to keywords....

[beachroad]

Replying to anonymous:

Replying to CuriousCurmudgeon:

Oops. I forgot to login before add a comment..