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

[JENKINS-32623] Use real URL for user #3046

Merged
merged 2 commits into from Oct 2, 2017

Conversation

@daniel-beck
Copy link
Member

commented Sep 26, 2017

See JENKINS-32623.

No tests because not worth it.

Untested so far, will give the PR build a try.

Proposed changelog entries

  • Bug: Fix link from build cause to user in case of unusual user names.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
@oleg-nenashev
Copy link
Member

left a comment

http://javadoc.jenkins.io/hudson/model/User.html#get-java.lang.String-boolean-java.util.Map- should be used in this case. Actually I would argue that User.get(String) should be deprecated at all since it causes much confusion

@@ -20,5 +20,5 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

started_by_user=Started by user <a href="{2}/user/{0}">{1}</a>
started_by_user=Started by user <a href="{2}/{0}">{1}</a>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 26, 2017

Member

Does it generate the correct number of slashes in URL?

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Sep 26, 2017

Author Member

It does not include a leading slash, so, yes.

@@ -410,6 +412,11 @@ public String getUserName() {
return userId == null ? "anonymous" : User.get(userId).getDisplayName();
}

@Restricted(DoNotUse.class) // for Jelly
public String getUserUrl() {
return userId == null ? null : User.get(userId).getUrl();

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 26, 2017

Member

Should be http://javadoc.jenkins.io/hudson/model/User.html#get-java.lang.String-boolean-java.util.Map- instead. We do not want to occasionally re-create user instances in the case of race conditions (e.g. accessing page with UserIdCause in parallel with user deletion) 🐛

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Sep 26, 2017

Author Member

getUserName (5 lines above) does it already, so this PR doesn't make it worse.

@oleg-nenashev
Copy link
Member

left a comment

Screw these old getUser() APIs, I will burn them with fire at some point.

If you are fine with fixing them in this class while you are, it would be nice. But I agree it's not mandatory.

@daniel-beck

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

@oleg-nenashev Thanks. The only reason I bothered is that this was so straightforward, add a bunch of cleanup duty on top and I don't have the time 🙁

@daniel-beck daniel-beck merged commit 4109c5b into jenkinsci:master Oct 2, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.