Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10828 closed defect (fixed)

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

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: Ryan J Ollos <ryan.j.ollos@…>
Priority: normal Milestone: 1.0.1
Component: ticket system Version: 1.0dev
Severity: normal Keywords: KeywordsField, Keywords
Cc: Branch:
Release Notes:

Fixed ticket property change descriptions like abc, def added; abc removed.

API Changes:
Internal Changes:

Description

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.

Attachments (0)

Change History (12)

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

Keywords: KeywordsField added

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

Keywords: KeywordsField added

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

Keywords: Keywords added

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

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.

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

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

     
    18001800            if not (Chrome(self.env).show_email_addresses or
    18011801                    'EMAIL_VIEW' in req.perm(resource_new or ticket.resource)):
    18021802                render_elt = obfuscate_email_address
     1803        elif field == 'keywords':
     1804            old_list = [old.strip(',; ') for old in old_list]
     1805            new_list = [new.strip(',; ') for new in new_list]
    18031806        if (old_list, new_list) != (None, None):
    18041807            added = [tag.em(render_elt(x)) for x in new_list
    18051808                     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, kw4kw1 kw3, kw4 results in Keywords changed from kw1, kw3, kw4 to kw1 kw3, kw4.

comment:6 by Peter Suter, 8 years ago

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

Untested, but how about:

  • trac/ticket/web_ui.py

    diff -r 5f81f6b2bc10 trac/ticket/web_ui.py
    a b  
    17911791                diff = tag.a(_("diff"), href=href)
    17921792                rendered = tag_("modified (%(diff)s)", diff=diff)
    17931793        elif type_ == 'text' and field_info.get('format') == 'list':
    1794             old_list, new_list = old.split(), new.split()
     1794            old_list = re.split(r'[;,\s]+', old)
     1795            new_list = re.split(r'[;,\s]+', new)
    17951796            sep = ' '
    17961797
    17971798        # per name special rendering of diffs

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

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

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.

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

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.

in reply to:  5 comment:9 by Ryan J Ollos <ryan.j.ollos@…>, 8 years ago

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, kw4kw1 kw3, kw4 results in Keywords changed from kw1, kw3, kw4 to kw1 kw3, kw4.

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

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

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

in reply to:  8 comment:11 by Peter Suter, 8 years ago

Milestone: 1.0.1
Resolution: fixed
Status: newclosed

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].

comment:12 by Peter Suter, 8 years ago

Owner: set to Ryan J Ollos <ryan.j.ollos@…>
Release Notes: modified (diff)

Modify Ticket

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