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

Introduce the new UserIdCause constructor, which accepts userId as an argument. #3162

Merged
merged 2 commits into from Dec 14, 2017

Conversation

3 participants
@oleg-nenashev
Member

oleg-nenashev commented Nov 27, 2017

I hit the issue with the missing constructor while working on jenkinsci/job-restrictions-plugin#20, ant actually it was not the first time I hit it. So I decided to add a new API method and to document the behavior while I was around.

Proposed changelog entries

  • Developer: Introduce the new hudson.model.Cause.UserIdCause(String userId) constructor, which allows creating causes for custom IDs without switching the user context.
  • ...

Submitter checklist

No issue and no tests, just a minor developer update

Desired reviewers

@reviewbybees

@oleg-nenashev oleg-nenashev changed the title from Introduce UserIdCause constructor, which accepts userId as an argument. to Introduce the new UserIdCause constructor, which accepts userId as an argument. Nov 27, 2017

@recampbell recampbell requested a review from jglick Nov 29, 2017

@@ -436,7 +455,7 @@ public String getShortDescription() {
public void print(TaskListener listener) {
listener.getLogger().println(Messages.Cause_UserIdCause_ShortDescription(
// TODO better to use ModelHyperlinkNote.encodeTo(User), or User.getUrl, since it handles URL escaping
ModelHyperlinkNote.encodeTo("/user/"+getUserId(), getUserName())));
ModelHyperlinkNote.encodeTo("/user/"+getUserIdOrUnknown(), getUserName())));

This comment has been minimized.

@daniel-beck

daniel-beck Dec 5, 2017

Member

Shouldn't this use getUserUrl to begin with?

This comment has been minimized.

@jglick

jglick Dec 6, 2017

Member

As in my comment above?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 6, 2017

Member

I just decided not to tough it since it does not make the thing worse

@jglick

The constructor seems fine in general, though I have strong reservations about the proposed downstream PR (and it is unclear to me whether the test would still exist in the same form if it were rewritten).

Anyway, I think the only issue here is the hyperlinking, which is wrong. OTOH it was wrong before, so this is not making anything worse really.

@@ -436,7 +455,7 @@ public String getShortDescription() {
public void print(TaskListener listener) {
listener.getLogger().println(Messages.Cause_UserIdCause_ShortDescription(
// TODO better to use ModelHyperlinkNote.encodeTo(User), or User.getUrl, since it handles URL escaping
ModelHyperlinkNote.encodeTo("/user/"+getUserId(), getUserName())));
ModelHyperlinkNote.encodeTo("/user/"+getUserIdOrUnknown(), getUserName())));

This comment has been minimized.

@jglick

jglick Dec 6, 2017

Member

As in my comment above?

@Nonnull
private String getUserIdOrUnknown() {
return userId != null ? userId : User.getUnknown().getId();

This comment has been minimized.

@jglick

jglick Dec 6, 2017

Member

Useless, since if userId == null we just want to suppress the hyperlinking.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 6, 2017

Member

Yes, maybe it makes sense to handle it differently. It is a private method anyway

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Dec 10, 2017

@jglick I have created https://issues.jenkins-ci.org/browse/JENKINS-48467 and referenced it from the code. Is it fine to go now assuming that the new API is useful in general? I agree that the downstream PR is not a right implementation of the issue fix, but IMHO it still offers a use-case for test environments at least

@oleg-nenashev oleg-nenashev requested a review from jglick Dec 10, 2017

@jglick

jglick approved these changes Dec 11, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Dec 12, 2017

@oleg-nenashev oleg-nenashev merged commit 14605fe into jenkinsci:master Dec 14, 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