Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add patchset-notified event #33

Merged
merged 1 commit into from
May 4, 2015

Conversation

rinrinne
Copy link
Contributor

This patch adds patchset-notified event.

It is a extended event defined by 'Gerrit Notify PatchSet Plugin'.
https://github.com/rinrinne/gerrit-notify-patchset

This patch adds patchset-notified event.

It is a extended event defined by 'Gerrit Notify PatchSet Plugin'.
https://github.com/rinrinne/gerrit-notify-patchset
public void fromJson(JSONObject json) {
super.fromJson(json);
if (json.containsKey(NOTIFIER)) {
this.account = new Account(json.getJSONObject(NOTIFIER));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure overwriting the account field is "good". But at the same time I'm not sure exactly what the account field is about. Is it the owner of the change or the user who initialized the event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

account filed was created by the below commit
dd351e2

All events except ref-updated has account attribute, Key name is e.g. abandoner, uploader, submitter, and so on. By the above patch, those were refactored then stored into one field named account.

This account is the person who did something operation which causes event on Gerrit. In my plugin case, it is the person who pushed Notify button on revision page.

So I think it is proper to store account attribute in notifier key into account field.

Just in case, I confirmed no one sets value to account field.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, great

@rsandell
Copy link
Collaborator

I think we can go with this for now, its a bit on the edge since its not a "Gerrit core" event type. As we've discussed before; some kind of extensability to GerritEventType should be made, but this could work for now.

I would like to hold off on merging this though until 2.13 of the Gerrit Trigger is out of beta. Or maybe I should just start branching...

@rinrinne
Copy link
Contributor Author

Agree:+1:

@rsandell rsandell merged commit 2a35e09 into sonyxperiadev:master May 4, 2015
rsandell added a commit that referenced this pull request May 4, 2015
@rsandell
Copy link
Collaborator

rsandell commented May 4, 2015

Released in 2.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants