New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 5329 link

Starred by 2 users

Issue metadata

Status: Released
Owner:
Closed: Oct 2017
Cc:
Components:



Sign in to add a comment

Support find-owners plugin

Project Member Reported by vikt...@google.com, Jan 24 2017

Issue description

Please post links to the code, or list APIs used.
 

Comment 1 by chh@google.com, Jan 24 2017

Current find-owners plugin uses:

* post(REVISION_KIND, "find-owners").to(FindOwnersAction.class);
  in a RestApiModule.configure() function to add a post REST API.

* DynamicSet.bind(binder(), WebUiPlugin.class)
  .toInstance(new JavaScriptPlugin("find-owners.js"));
  in an AbstractModule.configure() function to add the JavaScript.

* FindOwersAction.getDescription() function to decide dynamically
  if the "Find Owners" button should be visible:
    return new Description()
        .setLabel("Find Owners")
        .setTitle("Find owners to add to Reviewers list")
        .setVisible(needFindOwners);

* The find-owners.js script uses pretty common pattern:

Gerrit.install(function(self) {
  function onFindOwners(c) {
    // Other helper functions use Gerrit.{get,post,delete,refresh} to
    // get/update informations from the gerrit server.
    const changeId = c.change._number;
    const message = c.revision.commit.message;
    function showFindOwnersResults(result) {
      ....
      // Other functions to create elements like c.button, c.hr, etc.
      c.popup(...)
    }
    c.call({change: changeId}, showFindOwnersResults);
    // or
    // self.get('change/' + changeId, showFindOwnersResults);
  }
  self.onAction('revision', 'find-owners', onFindOwners);
});

Project Member

Comment 2 by andyb...@chromium.org, Jan 24 2017

Status: Accepted (was: New)
Project Member

Comment 3 by jrn@google.com, May 4 2017

Labels: -Restrict-View-Google Hotlist-Migration
The code is at https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/static/find-owners.js.
Project Member

Comment 4 by jrn@google.com, May 4 2017

Components: plugins>PluginInfrastructure

Comment 5 by chh@google.com, Jul 5 2017

Do we now have a PolyGerrit design to support plugin buttons like the "Find Owners"?

Project Member

Comment 6 by vikt...@google.com, Jul 6 2017

PolyGerrit plugin API is still WIP.
I'll update this bug once it's ready for testing/integration.

Comment 7 Deleted

Comment 8 Deleted

Comment 9 by chh@google.com, Aug 29 2017

The find-owners plugin has been simplified to depend on fewer Gerrit APIs.
Not all APIs mentioned in #1 are used now, but the following are still needed:

(1) Some way to add a "Find Owners" button to the PolyGerrit change page.
(2) Gerrit JavaScript APIs:
  Gerrit.get
  Gerrit.post
  Gerrit.delete
  Gerrit.refresh

I found that self.{get,post} can be used instead of Gerrit.{get,post},
but self.delete is missing too.
Could you add those APIs in (2) first?
Thanks.

Project Member

Comment 10 by vikt...@google.com, Sep 5 2017

Delete API method added in https://gerrit-review.googlesource.com/c/gerrit/+/124870

For Gerrit.refresh(), see https://bugs.chromium.org/p/gerrit/issues/detail?id=6689
Full page refresh (https://developer.mozilla.org/en-US/docs/Web/API/Location/reload) can be used meanwhile, but it's quite slow, so we're trying to provide faster options.

Comment 11 by chh@google.com, Sep 6 2017

Thanks for adding Plugin.delete.

I have already changed find-owners plugin to fall back to
self.delete if Gerrit.delete is missing,
and location.reload if Gerrit.refresh is missing.

My code looks like this:

      (!!Gerrit.delete) ? Gerrit.delete(url, callback) :
          ((!!self.delete) ? self.delete('/../..' + url, callback) :
              httpError('DELETE ' + url, callback));

      (!!Gerrit.refresh) ? Gerrit.refresh() : location.reload();


'location.reload()' works for me now, although it is very slow.


self.delete does not work for me as implemented now in Plugin.delete.
I call Gerrit.delete with url like: /changes/50/reviewers/1000002
or self.delete with url like: /../../changes/50/reviewers/1000002
to delete reviewer 1000002 from change number 50.

When Plugin.delete checked the response.status, it got 400 and
then called Promise.reject instead of my callback function.

Could Plugin.delete be implemented like Plugin.get and Plugin.post?

  Plugin.prototype.delete = function(url, opt_callback) {
    return this._send('DELETE', url, opt_callback);
  }

That will work for me to replace the Gerrit.delete.
Thanks.

Project Member

Comment 12 by vikt...@google.com, Sep 6 2017

> Could Plugin.delete be implemented like Plugin.get and Plugin.post?

Yes, this is exactly how it's implemented (https://gerrit-review.googlesource.com/c/gerrit/+/124870). I'm not sure it's deployed on googlesource.com yet, though.

Comment 13 by chh@google.com, Sep 6 2017

I pulled the change in 124870 to build locally and it did not work due to returned status 400. After that, I modified it in my local build to only
      return this._send('DELETE', url, opt_callback);
That worked.

Project Member

Comment 14 by vikt...@google.com, Sep 6 2017

Could you elaborate on this? I've re-tested self.delete method again and was able to use HTTP DELETE to remove a vote.

Isn't 400 an error code? Let's chat about this.
Project Member

Comment 15 by vikt...@google.com, Sep 15 2017

Comment 16 by chh@google.com, Sep 15 2017

Thank you. The only feature missing now is some way to add a "Find Owners" button to the PolyGerrit change page.

Comment 17 by srhines@google.com, Oct 18 2017

What is preventing the button from being added to the PolyGerrit change page? This is blocking usage on Android, which is increasingly dependent on PolyGerrit (instead of the legacy UI).
Project Member

Comment 18 by vikt...@google.com, Oct 19 2017

Sorry for the delay. Unfortunately, as of now there is no direct replacement for the old API in question. Here are some hacks, if that's a priority - but clearly, this would need cleanup later if used.

Overall, there is no clear path at the moment for developing advanced plugins that target both GWT and PG. We're working to provide one, but it's WIP.

There is undocumented API that can be used right now, for PolyGerrit only.
https://gerrit.googlesource.com/gerrit/+/master/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api.js

Here's the sample code:
var buttonId = self.changeActions().add('change', 'Find Owners');
self.changeActions().addTapListener(buttonId, onFindOwners);

Downside is that there's no context available, and to get it another non-documented method should be used:

self.on('showchange', function(change, revision) {
  // 
});

Project Member

Comment 19 by vikt...@google.com, Oct 25 2017

Update: I've tested find-owners with https://gerrit-review.googlesource.com/c/gerrit/+/135930, and looks like it's working.
Project Member

Comment 20 by jrn@google.com, Oct 27 2017

Components: plugins>find-owners
Project Member

Comment 21 by jrn@google.com, Oct 27 2017

Status: Submitted (was: Accepted)
https://gerrit-review.googlesource.com/c/plugins/find-owners/+/136972

Comment 22 by chh@google.com, Oct 27 2017

Thanks for the changes. We should see after next week's Android Gerrit server push the new "Find Onwers" item in the "MORE" pull down action list.


Project Member

Comment 23 by beckysiegel@google.com, Nov 1 2017

Status: Released (was: Submitted)
Labels: FixedIn-2.16

Sign in to add a comment