904988 - create an ember extension with bespoke methods

Status: RESOLVED FIXED

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

Description

[:glob ✱]

create an ember extension with bespoke methods, to minimise the number of calls required, and to allow rapid turn-around on changes to these methods (ie. outside of upstream requirements).

we initially need two new methods, with the goal of reducing the number of calls required to create or edit a bug to one:

create
  input : product name (or id?)
  output: field data for the authenticated user
    - editable fields only
    - all field data (mandatory, multi-select values, field type, etc)
    - needs to include all custom and tracking fields

show
  input: bug_id, last-updated (optional)
  output: fields with values
    - visible fields only
    - editability data (can-edit, enabled, disabled)
    - multi-select values
    - comments
    - attachments metadata
    - mandatory fields, field type
    - server-side management of "disabled but set value"s
    - if last-updated is provided, only return fields/comments/attachments set since the provided datetime

Comment 1

[David Lawrence [:dkl]]

Attachment: 904988_1.patch (not done)

What I have do far. Will ping you to discuss some questions I have regarding data structure.

Comment 2

[:glob ✱]

Comment on attachment 796027 [details] [diff] [review]
904988_1.patch (not done)

dkl and i had a long chat yesterday about this patch, where we sorted out a few issues with optimising the returned structure to avoid returning redundant data and structures.

Comment 3

[David Lawrence [:dkl]]

Attachment: 904988_2.patch

New patch with data structure returned as discussed. Let me know if this is path you were envisioning and I will finish documenting.

dkl

Comment 4

[:glob ✱]

Comment on attachment 797223 [details] [diff] [review]
904988_2.patch

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

this looks like it's on the right track.
it's ok hold off on doing the documentation until we get an r+ on everything else.

::: extensions/Ember/lib/WebService.pm
@@ +20,5 @@
> +use Bugzilla::Constants;
> +use Bugzilla::Error;
> +use Bugzilla::Util qw(trick_taint);
> +
> +use Bugzilla::Extension::Ember::FakeBug;

this is missing from the patch, so unfortunately i can't test it to see the actual response

@@ +174,5 @@
> +        # These fields cannot be edited by user so should be omitted
> +        foreach my $non_edit (qw(assignee_accessible bug_id commenter creation_ts delta_ts
> +                                 everconfirmed qacontact_accessible reporter)) {
> +            @field_objs = grep($_->name ne $non_edit, @field_objs);
> +        }

these fields need to be returned for show() calls, but not for create() calls.

Comment 5

[David Lawrence [:dkl]]

Attachment: 904988_3.patch

Thanks for the initial review

1. Added missing FakeBug.pm modules
2. Improved filtering of inactive/active fields/values.
3. Improved return data when only looking for changes made since a last date (last_updated)

I will go ahead and push this as well to https://bugzilla-dev.allizom.org so ebryn can start looking at it as well.

dkl

Comment 6

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.2-dev
modified Bugzilla/WebService/Bug.pm
added extensions/Ember
added extensions/Ember/Config.pm
added extensions/Ember/Extension.pm
added extensions/Ember/lib
added extensions/Ember/lib/FakeBug.pm
added extensions/Ember/lib/WebService.pm
modified extensions/TrackingFlags/lib/Flag.pm
Committed revision 8536.

Comment 7

[Erik Bryn]

What's keeping the show API from working unauthenticated?

Comment 8

[Erik Bryn]

Also, is there a way to tell if the bug itself is editable by the user? I see flags on every field, but it'd be nice to have an aggregate flag which would tell me whether I should bother showing an edit button.

Comment 9

[:glob ✱]

(In reply to Erik Bryn from comment #8)
> Also, is there a way to tell if the bug itself is editable by the user?

as per bug 897835 comment 1, that concept simply doesn't exist in bugzilla.  editibility is on a per field and per value level.  it's very rare for a bug to not allow any edits (including adding comments) from authenticated users, however if you did want to hide the button you should be able to do it by looking at the data returned by these methods.

Comment 10

[:glob ✱]

Comment on attachment 797910 [details] [diff] [review]
904988_3.patch

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

the following applies to create() only:

- there's no need to return is_obsolete on fields - if a field is obsolete it shouldn't be returned
  - the same applies to field values.
- field values have a can_edit property - this can probably be dropped
- field values return an is_active property - this should be dropped as inactive values shouldn't be returned
- is_on_bug_entry property isn't required to be returned, instead a field should be not be returned if this is false
- the following fields do not need to be returned:
  - days_elapsed
  - reporter_accessible
  - cclist_accessible
- timetracking fields need to honour the visibility of those fields to the current user (currently they are always returned)
- remove the "product" property completely
  - return values for version and milestones under their respective fields
  - there's no need to return complete information about components - the component names are already returned as part of the component field, and i would expect ember to cache component descriptions, etc

the following applies to show() only:

- there's fields which are not editable, such as bug_id, which are flagged as can_edit=true
- i don't think it's worth returning the full list of classifications and products under their respective fields.  while this is inconsistent, these fields are unlikely to change and add significantly to the payload size
- version and milestone fields are missing their values
- as far as i can tell, returning can_edit for values isn't required

::: extensions/Ember/lib/WebService.pm
@@ +77,5 @@
> +    my ($self, $params) = @_;
> +    my (@fields, $attachments, $comments, $data);
> +    my $dbh = Bugzilla->dbh;
> +
> +    Bugzilla->login(LOGIN_REQUIRED);

login shouldn't be required

@@ +105,5 @@
> +            @fields = $self->_get_fields($bug, $updated_fields);
> +        }
> +
> +        # Find any comments created since the last_updated date
> +        $comments = $self->comments({ ids => $bug_id,  new_since => $last_updated });

nit: two spaces before new_since

@@ +137,5 @@
> +    # Place the fields current value along with the field definition
> +    foreach my $field (@fields) {
> +        $field->{current_value} = delete $bug_hash->{$field->{api_name}}
> +                                  || delete $bug_hash->{$field->{name}}
> +                                  || '';

these need to be // instead of || to accommodate fields with 0 in them.

Comment 11

[Erik Bryn]

(In reply to Byron Jones ‹:glob› from comment #10)
> - i don't think it's worth returning the full list of classifications and
> products under their respective fields.  while this is inconsistent, these
> fields are unlikely to change and add significantly to the payload size

I would like it if we could leave these in the show response for now. We can remove them in the future if need be.

Comment 12

[Erik Bryn]

Another thing: it appears that some fields with values have a default initial value. For example, severity defaults to "normal" and milestone and priority default to "--". Is this something configurable? If so, can it be returned in the create API response?

Comment 13

[David Lawrence [:dkl]]

Attachment: 904988_4.patch

Thanks glob. This should address your comments as well as ebryns comment as well.

dkl

Comment 14

[David Lawrence [:dkl]]

Updated https://bugzilla-dev.allizom.org with latest patch. Should be there shortly.

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

dkl

Comment 15

[:glob ✱]

Comment on attachment 799092 [details] [diff] [review]
904988_4.patch

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

create()
* all tracking flags are missing :(
* no need to return the classification field, as it can't be set
* bug_group should iterate valid groups as values
* maybe remove 'tag' completely?

show()
* can_edit is incorrectly set to true for classification
* we're returning both "bug_group" and "groups"
  - i don't know what the difference is :)
  - i would expect a single groups field to be returned
    - current groups in a current_value array
    - available groups in a values array
* time tracking fields still returned
  - deadline
  - percentage_complete
* maybe remove 'tag' completely?
* inactive tracking flags are begin returned
  - perhaps we should map "---" to an empty string?

::: extensions/Ember/lib/WebService.pm
@@ +228,5 @@
> +        # for attachments.*, etc.
> +        next if $field_name =~ /\./;
> +
> +        # Remove time tracking fields if the user is privileged
> +        next if ($field_name =~ /_time$/ && !Bugzilla->user->is_timetracker);

this needs to be an explicit list of fields rather than a regex -- you're missing fields and could match non-timetracking fields (esp custom fields).

Comment 16

[David Lawrence [:dkl]]

Attachment: 904988_5.patch

Comment 17

[:glob ✱]

Comment on attachment 800387 [details] [diff] [review]
904988_5.patch

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

r=glob

erik, once this lands, please file new bugs for any changes.

::: extensions/Ember/lib/WebService.pm
@@ +141,5 @@
> +        my $updated_attachments =
> +            $dbh->selectcol_arrayref('SELECT attach_id FROM attachments
> +                                       WHERE (creation_ts > ? OR modification_time > ?)
> +                                             AND bug_id = ?',
> +                                     undef, ($last_updated, $last_updated, $bug->id));

unlike $self->comments(), which skips private comments if the user isn't in the insider group, $self->attachments() throws an auth_failure error :(

you'll have to include a check for attachments.isprivate in your sql to avoid this.

Comment 18

[Erik Bryn]

It appears that flags are still missing in the create API response.

The api_name for `longdesc` is also coming back wrong, I believe it should be `description` as that is the name the Bug.create API expects.

It seems like we can omit the `name` field from the response as well, I don't believe that is useful to the frontend. Then `api_name` could be renamed to `name`.

Comment 19

[David Lawrence [:dkl]]

(In reply to Erik Bryn from comment #18)
> It appears that flags are still missing in the create API response.
> 
> The api_name for `longdesc` is also coming back wrong, I believe it should
> be `description` as that is the name the Bug.create API expects.
> 
> It seems like we can omit the `name` field from the response as well, I
> don't believe that is useful to the frontend. Then `api_name` could be
> renamed to `name`.

I have pushed changes now to bugzilla-dev to address the above. Please give feedback.

dkl

Comment 20

[David Lawrence [:dkl]]

Attachment: 904988_6.patch

Updated patch. Rolling forward r+.

Comment 21

[Erik Bryn]

I'm seeing more flags in the create API response than I see on the existing bug entry form. For example, for the bugzilla.mozilla.org product (https://bugzilla-dev.allizom.org/enter_bug.cgi?format=__default__&product=bugzilla.mozilla.org&short_desc=testing) , I see `needinfo`, `sec-review`, and `sec-bounty`. The create API response contains those three but also `review` and `feedback`.

Comment 22

[David Lawrence [:dkl]]

Attachment: 904988_7.patch

Sorry to hit you with another patch :( This one adds the ability to create a new bug with flags and to also update flags of a current bug. I added two new methods to the Ember extension called Ember.create_bug and Ember.update_bug. POST /rest/ember/bug and PUT /rest/ember/bug/<bugid> respectively. I am basically wrapping the normal Bug.create and Bug.update methods.

For creating, along with the normal key/value pairs needed by Bug.create, you can pass a new key called new_flags. You will need to know the type_id of the flag(s) you want to create. You can get the type_id(s) by looking at the possible flags from GET /rest/ember/create/<product>. The format is like this:

{ 
  "new_flags": [ 
    { "type_id": 800, "status": "?", "requestee": "[e-mail]" } 
  ] 
}

For updating a bug, you do the same for creating "new" flags by passing something like the above to PUT /rest/ember/bug/<bugid>. To update existing flags you will need to know the id of the flag you want to update which you can get from looking at the current_value list for flag in GET /rest/ember/show/<bugid>. To update a current flag you need the following data. To clear a flag use 'X' as the status value.

{
  "flags": [
    { "id": 894798, "status": "X" }
  ]
}

status can be either ?, +, -, or X to clear. If you want to set more than one flag that is multiple, then just add another one to new_flags with the same type_id, etc.

Let me know if you have any questions.

dkl

Comment 23

[:glob ✱]

Comment on attachment 802011 [details] [diff] [review]
904988_7.patch

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

::: extensions/Ember/lib/WebService.pm
@@ +252,5 @@
> +    $bug->set_flags($old_flags, $new_flags);
> +    my ($removed, $added) = Bugzilla::Flag->update_flags($bug, $old_bug, $bug->delta_ts);
> +    if ($removed || $added) {
> +        $result->{bugs}->[0]->{changes}->{'flagtypes.name'} = [$removed, $added];
> +    }

you need to call $bug->update() not just update_flags().

the current code doesn't insert bug_activity entires, nor does it trigger the correct hooks (eg. it's possible to set needinfo to + with this method).

Comment 24

[David Lawrence [:dkl]]

Attachment: 904988_8.patch

Ok, changed the format for flags to match what is being proposed upstream for less headache later when switching over.

For both create and update, you only pass in a key called 'flags' which is a list of data structures. For example:

POST /rest/ember/bug

{ 
  "flags": [ 
    { "type_id": 800, "status": "?", "requestee": "[e-mail]" } 
  ] 
}

PUT /rest/ember/bug/<bugid>

{
  "flags": [
    { "id": 894798, "status": "X" },
    { "type_id": 800, "status": "?", "requestee": "[e-mail]" }
  ]
}

As you can see for editing, both setting new flags and updating old flags are part of the same 'flags' list. But for editing an existing flag, you must specify 'id' of the flag, otherwise others with 'type_id' will be interpreted as new flags. 

One bug which I will work out in the next iteration is that emails are not currently sent for the flag changes since they occur after the main bug update and main bug update also sends the email at that time. I could do send_changes() again after the flag update but that would result in two emails per bug update request and that is not desirable. I will try to figure out how to do this better but this will get you going as email shouldn't hurt your work as it is done server side.

dkl

Comment 25

[:glob ✱]

Comment on attachment 802376 [details] [diff] [review]
904988_8.patch

this is much cleaner, but you can still set a needinfo flag to +, which indicates the correct hooks are not being triggered.

Comment 26

[David Lawrence [:dkl]]

Committed 904988_6.patch which is the read-only support for this. I will work on the flag modification updates as part of a new bug or push forward the upstream bug 756048.

Committing to: bzr+ssh://[e-mail]/bmo/4.2
modified Bugzilla/WebService/Bug.pm
added extensions/Ember
added extensions/Ember/Config.pm
added extensions/Ember/Extension.pm
added extensions/Ember/lib
added extensions/Ember/lib/FakeBug.pm
added extensions/Ember/lib/WebService.pm
modified extensions/TrackingFlags/lib/Flag.pm
Committed revision 8992.

Comment 27

[Erik Bryn]

I'm still seeing the issue I mentioned in comment 21. More flags are returned in the create API than are selectable in the existing bug creation form.

Comment 28

[:glob ✱]

(In reply to Erik Bryn from comment #27)
> I'm still seeing the issue I mentioned in comment 21. More flags are
> returned in the create API than are selectable in the existing bug creation
> form.

sorry that was missed -- as this bug has been resolved/fixed and deployed, i've filed bug 915197 to track that issue.