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

Update avatar URL for a user + refactoring #15

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

jhaverhals
Copy link

Add domainname to avatar URL + refactoring so that the URL is now
defined on a single location.

Add domainname to avatar URL + refactoring so that the URL is now
defined on a single location.
public String setAvatarURL(String userName)
{
/* Below URL is of the Crucible ( codereview ) server */
this.avatarURL = "http://srv-ind-scrat.vanenburg.com:8060/avatar/" + userName;
Copy link

@astellingwerf astellingwerf May 18, 2016

Choose a reason for hiding this comment

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

Shouldn't the ?s=number be part of this code as well? That'd make it trivial to switch to another avatar source (like Jira).

Copy link
Author

Choose a reason for hiding this comment

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

The required size of the avatar (requested with the ?s=number GET parameter) is dependent on the location where it is used. Currently three times 32, and one time 40.
If you really think it is valuable to make that split right now, maybe 'avatarURLSmall' and avatarURLMedium' or something like that can be introduced for that.

@cbos
Copy link
Owner

cbos commented Sep 6, 2016

  • Can you update the title inline with changes made. The changes about the avatar url are reverted.
  • Is the json output request from Jenkins different then before? Or does this leave out the stuff which is not required for parsing the results? I see that there is no unit test for the TestReportParser, I think it is good to add a sample response and add that with a testcase like done for the other parsers. Then you have at least a reference response which is valid.

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.

3 participants