Modifications to Keyword field can show 'removed' in comment even if no keywords were removed

[Ryan J Ollos <ryan.j.ollos@…>]

To reproduce:

1. Add a keyword abc
2. Append , def
3. Notice that the comment is abc, def added; abc removed

This is similar to the issue that existed with the Cc field in #8597.

[Ryan J Ollos <ryan.j.ollos@…>]

I'm not sure why we have a identical comment:1 and comment:2. I'm fairly sure I only hit submit once. Might be an issue to keep on the lookout for.

[Ryan J Ollos <ryan.j.ollos@…>]

The solution I see to this problem is to strip off commas, semicolons and whitespace, as is done for the CC list, and add the entries to the database as a single-whitespace separated list. This might cause issues for users of the ​th:KeywordSuggestPlugin with its multipleseparator option, but I don't see a much cleaner way to approach the issue, unless we wish to add a keyword separator option to Trac. Another factor is that it could be argued that it is confusing and inconsistent to fixup the CC list and always use a comma separator there, but use a whitespace separator for the keywords list.

If we don't wish to change the existing behavior, then the following patch will not alter the data storage, but will fix most of the added/removed issue:

trac-trunk/trac/ticket/web_ui.py

@@ -1800 +1800 @@
             if not (Chrome(self.env).show_email_addresses or 
                     'EMAIL_VIEW' in req.perm(resource_new or ticket.resource)):
                 render_elt = obfuscate_email_address
+        elif field == 'keywords':
+            old_list = [old.strip(',; ') for old in old_list]
+            new_list = [new.strip(',; ') for new in new_list]
         if (old_list, new_list) != (None, None):
             added = [tag.em(render_elt(x)) for x in new_list 
                      if x not in old_list]

This helps with adds/removes from the ends of the lists, but doesn't help with changes within the list. For example, removing a comma within a list: kw1 kw3, kw4 → kw1 kw3, kw4 results in Keywords changed from kw1, kw3, kw4 to kw1 kw3, kw4.

[Peter Suter]

I think TracTicketsCustomFields with format=list should be handled identical to keywords.

Untested, but how about:

trac/ticket/web_ui.py

@@ -1791 +1791 @@
                 diff = tag.a(_("diff"), href=href)
                 rendered = tag_("modified (%(diff)s)", diff=diff)
         elif type_ == 'text' and field_info.get('format') == 'list':
-            old_list, new_list = old.split(), new.split()
+            old_list = re.split(r'[;,\s]+', old)
+            new_list = re.split(r'[;,\s]+', new)
             sep = ' '
 
         # per name special rendering of diffs

(We previously discussed a keyword separator option and decided against it.)

[Ryan J Ollos <ryan.j.ollos@…>]

Yeah, that seems wise. I'll take a look as to whether we have test case coverage for this already, or if some tests should be added.

[Ryan J Ollos <ryan.j.ollos@…>]

It looks like we need a minor addition, to avoid entries such as:

Keywords abc def added; removed

The issue is:

>>> new = ''
>>> re.split(r'[;,\s]+', new)
['']

and some of the logic downstream is expecting an empty list in the case that new or old are empty strings. I'll address this in the patch.

It looks like all of the tests for ticket.web_ui exist in functional.py, so I added a test for the list-type field property change rendering behavior in TestTicketCustomFieldTextListFormatRenderedChanges. The name is too long, but I tried to pattern it after the existing test case name. An alternative approach would be to include the test logic in the existing TestTicketCustomFieldTextListFormat test case. I could merge the test cases if that would be more desirable.

The test and psuter's version of the fix with minor modification can be found in ​47872f79. I'd particularly appreciated feedback on the functional tests, as it will influence my work in #5658.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

This helps with adds/removes from the ends of the lists, but doesn't help with changes within the list. For example, removing a comma within a list: kw1 kw3, kw4 → kw1 kw3, kw4 results in Keywords changed from kw1, kw3, kw4 to kw1 kw3, kw4.

This contained a typo. It should read: kw1, kw3, kw4 → kw1 kw3, kw4 results in Keywords changed from kw1, kw3, kw4 to kw1 kw3, kw4.

[Ryan J Ollos <ryan.j.ollos@…>]

One other very minor thing that is bothering me is that the entries in a list can be rendered in the ticket property box without whitespace separators.

For example, keywords = abc, def,ghi renders as:

abc, def,ghi

but I think we should fixup the list so that there is always whitespace between the entries, even when only commas or semicolons have been entered as separators.

abc, def, ghi

I've implemented this change in ​84ceca21. I'd even favor dropping the comma or semicolon separator when displaying, so that abc, def,ghi results in:

abc def ghi

[Peter Suter]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

I added a test for the list-type field property change rendering behavior in TestTicketCustomFieldTextListFormatRenderedChanges. The name is too long, but I tried to pattern it after the existing test case name.

I renamed it to RegressionTestTicket10828 although it's maybe not much of an improvement.

Personally I like the comma separators.

Applied in [11484], merged to trunk in [11485].