919199 - Ember.show API returns unchanged fields when called with last_updated param

Status: RESOLVED FIXED

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

Description

[Erik Bryn]

When I began to reimplement polling using the Ember.show API, I noticed that fields are returned that haven't changed.

For example: https://bugzilla-dev.allizom.org/rest/ember/show/432984?last_updated=2013-09-21T17:06:29Z

I would expect to see no fields returned, but `flags` and `groups` are returned.

Comment 1

[Erik Bryn]

Also, I would expect the `last_change_time` field to display if the bug was changed since the provided `last_update` time.

Ideally I should be able to take a previously cached bug response and apply the "diff response" (one made with `last_update` provided) and have the equivalent data that a new request would return.

Comment 2

[:glob ✱]

Attachment: 919199_1.patch

> Ideally I should be able to take a previously cached bug response and apply
> the "diff response"

we don't strictly do that, as we return possible values as well as the current value.

Comment 3

[Erik Bryn]

(In reply to Byron Jones ‹:glob› from comment #2)
> Created attachment 809734 [details] [diff] [review]
> 919199_1.patch
> 
> > Ideally I should be able to take a previously cached bug response and apply
> > the "diff response"
> 
> we don't strictly do that, as we return possible values as well as the
> current value.

Are you saying that the possible values shouldn't be merged? If so, that wasn't my intent. I was expecting to completely overwrite the values from the updated response.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 809734 [details] [diff] [review]
919199_1.patch

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

::: extensions/Ember/lib/WebService.pm
@@ +232,4 @@
>      my @fields;
>      foreach my $field (@field_objs) {
> +        $return_groups = 1 if $field->name eq 'bug_group';
> +        $return_flags  = 1 if $field->name eq 'flagtypes.name';

One issue with this method is that even if just attachment flags change, we still include all of the bug flags as well in the changed field list. We may need to somehow look for attach_id in bugs_activity in Ember.show and set some kind of flag to not include bug flags if just attachment flags have changed.

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 809734 [details] [diff] [review]
919199_1.patch

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

Setting to r- as discussed to differentiate between attachment flag changes and bug flag changes.

Comment 6

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #5)
> Setting to r- as discussed to differentiate between attachment flag changes
> and bug flag changes.

Also slight conflict with the other recent flag *clusions patch committed but nothing major.

dkl

Comment 7

[:glob ✱]

i'm finally circling around to this request.

currently the data returned for flags includes both bug and attachment flags as possible values for the 'flags' field.  dkl and i are not sure what the best course of action is here as it depends on how you're using the data returned.

erik, if someone updates just an attachment's flag, what do you want returned as the possible values for the flags field:

1. just return the changed value, do not include any other possible values
2. just return possible attachment flags, do not include any bug flags as possible values
3. return both attachment and bug flags as possible values

Comment 8

[Erik Bryn]

glob: i think the easiest thing for me would be #3, but i wish the flags field didn't contain both bug and attachment flags. what do you think about separating them into separate fields?

Comment 9

[:glob ✱]

(In reply to Erik Bryn from comment #8)
> glob: i think the easiest thing for me would be #3

cool, that's the easiest thing for me too :)

> but i wish the flags field didn't contain both bug and attachment flags.
> what do you think about separating them into separate fields?

that's possible; can you please file that as a new bug though?  that'll make it easier to track.

Comment 10

[:glob ✱]

Attachment: 919199_2.patch

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 820821 [details] [diff] [review]
919199_2.patch

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

r=dkl. Some extra fields are returned such as status and resolution even when no changes are made due to the inclusion of values from _bug_to_hash that do not match fielddefs entries but that is a different bug that I will file.

Comment 13

[Erik Bryn]

(In reply to David Lawrence [:dkl] from comment #12)
> Some extra fields are returned such as status and resolution even
> when no changes are made due to the inclusion of values from _bug_to_hash
> that do not match fielddefs entries but that is a different bug that I will
> file.

I wasn't seeing that before, is that a new issue caused by this patch?

Also, I believe the `last_change_time` field should be always returned if there were any changes, otherwise I don't know what time to poll from next.

Comment 14

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #12)
> Some extra fields are returned such as status and resolution even
> when no changes are made due to the inclusion of values from _bug_to_hash
> that do not match fielddefs entries but that is a different bug that I will
> file.

i can't replicate that issue, and the code looks correct.  yes, _bug_to_hash always includes those fields, but they aren't copied into the response as they will be missing from @fields.

(In reply to Erik Bryn from comment #13)
> Also, I believe the `last_change_time` field should be always returned if
> there were any changes, otherwise I don't know what time to poll from next.

i can confirm that last_change_time will always be returned if there are changes.

for clarity, if there are not any changes you'll get:
{
   "attachments" : [],
   "comments" : [],
   "fields" : [],
   "id" : "712225"
}

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/Ember/lib/WebService.pm
Committed revision 9141.

Comment 15

[Erik Bryn]

It appears to work correctly with comments, but I'm not seeing field changes returned. I change both whiteboard and assigned_to and the changes aren't shown. I can see them on the history page of the existing interface.

Not sure if this was an issue introduced by bug 938763.

Comment 16

[:glob ✱]

(In reply to Erik Bryn from comment #15)
> It appears to work correctly with comments, but I'm not seeing field changes
> returned. I change both whiteboard and assigned_to and the changes aren't
> shown. I can see them on the history page of the existing interface.

erik - please don't reopen bugs that have already been committed; it makes fixing issues impossible to track if the fix if someone else does it.

if testing after bug 938763 is committed still shows that issue, please file a new bug.