770417 - backport bug 169752 to bmo/4.2

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[:glob ✱]

i want to backport bug 169752 to bmo/4.2, but just the fix itself without the refactoring.

Comment 1

[:glob ✱]

Attachment: patch v1

also includes the fix from bug 772359

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 640534 [details] [diff] [review]
patch v1

Review of attachment 640534 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Bug.pm
@@ +3827,5 @@
> +                && ($attachid || 0) == ($operation->{'attachid'} || 0))
> +            {
> +                my $old_change = pop @$changes;
> +                $removed = $old_change->{'removed'} . $removed;
> +                $added = $old_change->{'added'} . $added;

Need to add the commas or the values are butted up against each other. Also this breaks linkification of bug ids in show_activity.cgi. 

Suggested change:

$removed = $old_change->{'removed'}
           ? $old_change->{'removed'} . ", $removed" 
           : $removed;
$added   = $old_change->{'added'}
           ? $old_change->{'added'} . ", $added"
           : $added;

Comment 3

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #2)
> Comment on attachment 640534 [details] [diff] [review]
> Need to add the commas or the values are butted up against each other.

this won't work, because we can't tell if a comma or a space was actually removed when reconstructing the string.

the splitting code tries to break on a space or a comma, but this isn't guaranteed.  if it's unable to find a space to split at, it'll split at the max length, and it won't remove a comma or space in that case, so reinserting one when reconstructing is incorrect.

try it with a long url.

Comment 4

[:glob ✱]

we guess could have different combining rules for different fields:

if (buglist) { join with comma } else if (url) { join with empty string } else { join with space }

Comment 5

[:glob ✱]

Attachment: patch v2

this revision takes a guess at a reasonable delimiter.

if it's sane, i'll update the patch on bug 772359.

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 640933 [details] [diff] [review]
patch v2

Review of attachment 640933 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Bug.pm
@@ +3827,5 @@
> +                && ($attachid || 0) == ($operation->{'attachid'} || 0))
> +            {
> +                my $old_change = pop @$changes;
> +                $removed = _join_activity_entries($fieldname, $old_change->{'removed'}, $removed);
> +                $added = _join_activity_entries($fieldname, $old_change->{'added'}, $added);

nit: line up the =

@@ +3868,5 @@
> +    if ($field eq 'dependson' || $field eq 'blocked') {
> +        if (substr($new_change, 0, 1) eq ',') {
> +            return $current_change . $new_change;
> +        } else {
> +            return $current_change . ',' . $new_change;

Why not ', ' (extra space). IMO that is nicer on the eyes.

@@ +3874,5 @@
> +    }
> +
> +    # Assume the url fields contain a single url, so don't insert a delimiter
> +    if ($field eq 'bug_file_loc' || $field eq 'see_also') {
> +        return $current_change . $new_change;

When are you not adding a space here? The URLs get butted against each other in XMLRPC and the WebUI.

          {
            'when' => '20120711T15:59:06',
            'who' => '[e-mail]',
            'changes' => [
                         {
                           'removed' => '',
                           'added' => 'http://fedora/stable/show_bug.cgi?id=666667http://fedora/stable/show_bug.cgi?id=666668',
                           'field_name' => 'see_also'
                         }
                       ]
          }

Comment 7

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #6)
> Why not ', ' (extra space). IMO that is nicer on the eyes.

will do.

> When are you not adding a space here? The URLs get butted against each other
> in XMLRPC and the WebUI.

those fields are supposed to contain a single url.  if we split a single long url there wouldn't have been a space in the original data, and inserting a space again would result in a broken url.

Comment 8

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #7)
> > When are you not adding a space here? The URLs get butted against each other
> > in XMLRPC and the WebUI.
> 
> those fields are supposed to contain a single url.  if we split a single
> long url there wouldn't have been a space in the original data, and
> inserting a space again would result in a broken url.

You can insert one to many URLs in the See Also (add) field. When I enter two distinct URLs I get an activity entry with the values separated by ', '.

And I originally entered them without a comma and just a space. So not sure how the back end code is dealing with URLs with spaces in the original entry.

dkl

Comment 9

[:glob ✱]

Attachment: patch v3

(In reply to David Lawrence [:dkl] from comment #8)
> And I originally entered them without a comma and just a space. So not sure
> how the back end code is dealing with URLs with spaces in the original entry.

urls cannot legally contain spaces, so i don't think we should worry about this.

> You can insert one to many URLs in the See Also (add) field. When I enter
> two distinct URLs I get an activity entry with the values separated by ', '.

no worries, i can insert ', ' when recombining the see_also.

bear in mind it's impossible to fix this in a way that won't generate malformed data, so we have to go for best fit.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 641396 [details] [diff] [review]
patch v3

Review of attachment 641396 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

::: Bugzilla/Bug.pm
@@ +3863,5 @@
> +    if ($current_change eq '') {
> +        return $new_change;
> +    }
> +
> +    # Buglists need the comma restored

# Buglists or see_also need the comma restored

@@ +3872,5 @@
> +            return $current_change . ', ' . $new_change;
> +        }
> +    }
> +
> +    # Assume the url fields contain a single url, so don't insert a delimiter

# Assume the bug_file_loc field contains ...

::: extensions/InlineHistory/Extension.pm
@@ +35,5 @@
>  
>      # build bug activity
>      my ($activity) = Bugzilla::Bug::GetBugActivity($bug_id);
> +
> +use Data::Dumper; die Dumper($activity);

remove on commit

Comment 11

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified Bugzilla/Bug.pm
Committed revision 8246.