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

Fix problem where approvals were not displayed in Query and Trigger #272

Merged
merged 1 commit into from Dec 9, 2015

Conversation

Projects
None yet
3 participants
@engycz
Copy link
Contributor

commented Dec 4, 2015

Fix problem where approvals were not displayed in Query and Trigger Gerrit Patches window for patchsets which already has Verified or Code-Review approval.
Approvals are displayed only for latest patchset.
Decrease log verbosity for GerritProjectListUpdater.

[FIXED JENKINS-31894]

@@ -220,7 +220,7 @@ private void tryLoadProjectList() {
}
}
try {
logger.info("Trying to load project list.");
logger.trace("Trying to load project list.");

This comment has been minimized.

Copy link
@scoheb

scoheb Dec 4, 2015

Contributor

Personally I like seeing these log messages in the log. As an admin, there is a sense of comfort when I see these ;)

This comment has been minimized.

Copy link
@engycz

engycz Dec 4, 2015

Author Contributor

I like it too but if I have set project list refresh interval to 60 seconds, the log is full of this information. This information is not an error or warning. Admin can create new Log Recoreder and set Logger to com.sonyericsson.hudson.plugins.gerrit or adjust loglevel verbosity for com.sonyericsson.hudson.plugins.gerrit in global log.
But OK, I will remove it from this PR.

This comment has been minimized.

Copy link
@scoheb

scoheb Dec 4, 2015

Contributor
                                                                                  You are right. The only one I care about is the 2nd one...                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        From: Jiří EngelthalerSent: Friday, December 4, 2015 07:20To: jenkinsci/gerrit-trigger-pluginReply To: jenkinsci/gerrit-trigger-pluginCc: Scott HebertSubject: Re: [gerrit-trigger-plugin] Fix problem where approvals were not displayed in Query and Trigger (#272)In src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritProjectListUpdater.java:

@@ -220,7 +220,7 @@ private void tryLoadProjectList() {
}
}
try {

  •        logger.info("Trying to load project list.");
    
  •        logger.trace("Trying to load project list.");
    

I like it too but if I have set project list refresh interval to 60 seconds, the log is full of this information. This information is not an error or warning. Admin can create new Log Recoreder and set Logger to com.sonyericsson.hudson.plugins.gerrit or adjust loglevel verbosity for com.sonyericsson.hudson.plugins.gerrit in global log.
But OK, I will remove it from this PR.

—Reply to this email directly or view it on GitHub.

@engycz engycz force-pushed the engycz:visible-approvals branch from 51943b5 to bc7a5a1 Dec 4, 2015

@scoheb

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2015

I do not see any tests that demonstrate this fixed behavior.

* @return the highest and lowest code review vote for the patch set.
*/
public HighLow getCodeReview(JSONObject res) {
return Approval.CODE_REVIEW.getApprovals(res);
public HighLow getCodeReview(JSONObject res, int patchSetNumber) {

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 7, 2015

Member

I get a bit nervous when I see public methods changing signature like this. Can the old signature be left and deprecated with some default value?

This comment has been minimized.

Copy link
@engycz

engycz Dec 8, 2015

Author Contributor

OK, I will do.

lowValue = Math.min(lowValue, approval);
} catch (NumberFormatException nfe) {
logger.warn("Gerrit is bad at giving me Approval-numbers!", nfe);
if (patchSet.has("number") && patchSet.has("approvals")) {

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 7, 2015

Member

why is the number actually needed? The currentPatchSet is always the current regardless of what number it has right?

This comment has been minimized.

Copy link
@engycz

engycz Dec 8, 2015

Author Contributor

It's because approvals are related only to the latest patchset and when selecting "Include All Patchsets", outdated patchsets should not have filled Verify and Code-Review column.

This comment has been minimized.

Copy link
@rsandell
@engycz

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2015

I will post tests and fixes soon.

@engycz engycz force-pushed the engycz:visible-approvals branch from bc7a5a1 to 774729b Dec 8, 2015

/*
* The MIT License
*
* Copyright 2010 Sony Ericsson Mobile Communications. All rights reserved.

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 9, 2015

Member

wrong copyright

This comment has been minimized.

Copy link
@engycz

engycz Dec 9, 2015

Author Contributor

I will fix it.


}).when(session).setAttribute(eq("result"), any());

action.doGerritSearch("status:open", gerritServerName, false, request, response);

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 9, 2015

Member

this test does not seem to do any verification at all.

This comment has been minimized.

Copy link
@engycz

engycz Dec 9, 2015

Author Contributor

It does test when session.setAttribute is invoked. This is only one result of action.doGerritSearch.
There are assertEquals.


}).when(session).setAttribute(eq("result"), any());

action.doGerritSearch("status:open", gerritServerName, true, request, response);

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 9, 2015

Member

this test does no verification as well.

This comment has been minimized.

Copy link
@engycz

engycz Dec 9, 2015

Author Contributor

It does test when session.setAttribute is invoked. This is only one result of action.doGerritSearch.
There are assertEquals.

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 9, 2015

Member

Ah, sorry I probably read it too fast :)

I guess I'm used to seeing some kind of verify(theMock).someMethod(expected, parameters) and got confused :)

Fix problem where approvals were not displayed in Query and Trigger G…
…errit Patches window for patchsets which already has Verified or Code-Review approval.

Approvals are displayed only for latest patchset.
Decrease log verbosity for GerritProjectListUpdater.

[FIXED JENKINS-31894]

@engycz engycz force-pushed the engycz:visible-approvals branch from 774729b to 957082c Dec 9, 2015

*/
// CS IGNORE VisibilityModifier FOR NEXT 2 LINES. REASON: JenkinsRule.
@Rule
public final JenkinsRule j = new JenkinsRule();

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 9, 2015

Member

Since you are doing mocks of everything else I doubt that you need this as it only adds startup time to the tests.
An alternative would be to actually use this, don't have any mock objects at all and verify that the html page is displayed correctly.

This comment has been minimized.

Copy link
@engycz

engycz Dec 9, 2015

Author Contributor

May be you are right. At this moment I don't have enough time to rework the test. If you know how, you can do it.

rsandell added a commit that referenced this pull request Dec 9, 2015

Merge pull request #272 from engycz/visible-approvals
Fix problem where approvals were not displayed in Query and Trigger

@rsandell rsandell merged commit c9f4f78 into jenkinsci:master Dec 9, 2015

1 check passed

Jenkins This pull request looks good
Details

@engycz engycz deleted the engycz:visible-approvals branch Dec 9, 2015

@engycz engycz restored the engycz:visible-approvals branch Dec 22, 2015

@engycz engycz deleted the engycz:visible-approvals branch Jan 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.