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

Admin monitor message: separation between content and display #3416

Merged
merged 2 commits into from May 4, 2018

Conversation

5 participants
@Wadeck
Contributor

Wadeck commented May 2, 2018

  • separate the anchors from the message in my recent monitors
  • add dismiss button for them
  • use p instead of br
  • put the version for the RootUrlNotSetMonitor and the HttpResponses new method

No issue related, @recena that points me to that problem.

Screenshots

For the root URL monitor message:
root_url_warning

For the CSRF monitor message:
csrf_warning

Proposed changelog entries

  • Allow the "CSRF protection missing" and "Root URL not set" administrative monitors to be dismissed directly from their warning messages.

Desired reviewers

@reviewbybees @recena

Better separation between content and display
- also put the version for the RootUrlNotSetMonitor and the HttpResponses new method

@Wadeck Wadeck changed the title from Better separation between content and display to Admin monitor message: separation between content and display May 2, 2018

@Wadeck Wadeck added the needs-review label May 2, 2018

@dwnusbaum dwnusbaum requested review from dwnusbaum and kuisathaverat May 2, 2018

}
// used by jelly to determined if it's a null url or invalid one
@Restricted(NoExternalUse.class)
public boolean isUrlNull(){
JenkinsLocationConfiguration loc = JenkinsLocationConfiguration.get();
return loc != null && loc.getUrl() == null;
return loc.getUrl() == null;

This comment has been minimized.

@dwnusbaum

dwnusbaum May 2, 2018

Member

If you don't need the null check on loc here than can you remove it from isActivated as well? Looks like it is @CheckForNull so I think you want to add it back.

This comment has been minimized.

@Wadeck

Wadeck May 2, 2018

Contributor

It's already the case for isActivated and the JenkinsLocationConfiguration.get() is @Nonnull.

This comment has been minimized.

@dwnusbaum

dwnusbaum May 2, 2018

Member

I was looking at an old version, this looks good to me. Sorry for the noise!

Please provide an accurate value in <a href="{0}/configure">Jenkins configuration</a>.
PR status update, and environment variables such as <code>BUILD_URL</code>.
actionToTake=Please provide an accurate value in {0}.
actionUrlContent=Jenkins configuration

This comment has been minimized.

@dwnusbaum

dwnusbaum May 2, 2018

Member

nit: add a newline

<div class="alert alert-warning">
${%warningMessage(rootURL)}
<form method="post" action="${rootURL}/${it.url}/disable">

This comment has been minimized.

@dwnusbaum

dwnusbaum May 2, 2018

Member

Is allowing this monitor to be dismissed important enough for a changelog entry?

This comment has been minimized.

@Wadeck

Wadeck May 2, 2018

Contributor

Don't think as it's already possible by passing throught the configuration page, it's just a shortcut

@recena

This comment has been minimized.

Contributor

recena commented May 3, 2018

Much better.

@recena

recena approved these changes May 3, 2018

@oleg-nenashev

👍

@dwnusbaum

This comment has been minimized.

Member

dwnusbaum commented May 3, 2018

@oleg-nenashev oleg-nenashev merged commit a03254b into jenkinsci:master May 4, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented May 4, 2018

@Wadeck please write a changelog entry for it

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented May 4, 2018

@oleg-nenashev here you are! 👍

@dwnusbaum

This comment has been minimized.

Member

dwnusbaum commented May 4, 2018

@Wadeck I edited it slightly for clarity!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment