[FIX JENKINS-54325] Add support for multiple SimplePageDecorators in … #3710

jvz · 2018-10-29T21:06:26Z

…login.jelly

Signed-off-by: Matt Sicker boards@gmail.com

See JENKINS-54325.

Proposed changelog entries

Internal: login.jelly and signup.jelly now support displaying all registered SimplePageDecorator extensions rather than just the top priority one.
Internal: the password field in the login page now has a matching id as its name (j_password) for easier access from the DOM.

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.)
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees @scherler @Wadeck

…login.jelly Signed-off-by: Matt Sicker <boards@gmail.com>

Wadeck

nit: you can @Deprecated the getSimplePageDecorator()

RC: The hudson/security/HudsonPrivateSecurityRealm/signup.jelly also requires a modification otherwise the login page will work but not the signup one ;)

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2018-10-30T21:13:46Z

@Wadeck Deprecated the old function, and updated signup.jelly

jvz · 2018-10-30T21:23:31Z

core/src/main/resources/hudson/security/HudsonPrivateSecurityRealm/signup.jelly

                                }
                            </script>
                        </j:if>
+                        <div class="footer">


I'm not so sure about the HTML structure here. My use case in paranoia-plugin is to insert a <script> element here, so the rendering doesn't matter too much. The default SimplePageDescriptor does not specify a footer fragment.

I am fine with that

Wadeck

🐝

scherler

Internal: login.jelly and signup.jelly now support displaying all registered SimplePageDecorator extensions rather than just the top priority one.

Actually there is a point behind to return only one. Your changes will provoke that all existing login pages would display 2 logos when a custom one is provided.

Did you test this?

If you are not provide a way to actually limit e.g. the logo to only one implementations I am not seeing the point to break the only one implementation approach.

jvz · 2018-11-05T16:58:32Z

I tested this mainly with a new plugin called Paranoia. The problem being that I can't provide any footer without losing the header, and I didn't want to copy the default headers and styles.

Perhaps the logo should only display the first one while the other includes can overlap?

scherler · 2018-11-05T19:49:21Z

Did you see https://jenkins.io/blog/2018/06/27/new-login-page/ ? There is no need to copy and paste but you can include the default ones.

Regarding the footer could the paranoia plugin not expose another extension point which aggregates all footers and then inject it as only one

The reason of only one is to have a clean design and prevent surprises in design when plugins are extending.

jvz · 2018-11-05T20:18:15Z

Are you suggesting an extension point in core? If so, I could redo this change as such to add some sort of footer decorator. Otherwise, I'm not sure what you mean. I'd like to have this plugin be able to work with other custom page decorators or still use the default one otherwise.

scherler · 2018-11-05T20:19:23Z

Not in core in your plugin, people using it can extend it.

jvz · 2018-11-05T20:26:06Z

But then I'm stuck with either an unstyled set of login/signup pages, or I'm copying simple-head.jelly and simple-header.jelly to my plugin which makes those pages unable to be overridden.

scherler · 2018-11-05T20:30:24Z

Hmm, do you want to have a quick hangouts/meet/... about it?

In the blog post I wrote:

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
  <link rel="stylesheet" href="${resURL}/css/simple-page.css" type="text/css" />
  <link rel="stylesheet" href="${resURL}/css/simple-page.theme.css" type="text/css" />
  <style>
    .simple-page .logo {
        background-image: url('${resURL}/plugin/YOUR_PLUGIN/icons/my.svg');
        background-repeat: no-repeat;
        background-position: 50% 0;
        height: 130px;
    }
  </style>
  <link rel="stylesheet" href="${resURL}/css/simple-page-forms.css" type="text/css" />
</j:jelly>

That is using the core/default styles and adding to it, would that not cover your case?

jvz · 2018-11-05T20:38:11Z

I mean I can do that, but this plugin isn't overriding the styles there. I'm injecting scripts into pages.

Edit: see my usage here: https://github.com/jenkinsci/paranoia-plugin/blob/master/src/main/resources/io/jenkins/plugins/paranoia/DisableLoginAutocompleteDecorator/simple-footer.jelly

scherler

As I see it you need simply provide src/main/resources/jenkins/model/DefaultSimplePageDecorator/simple-footer.jelly and inject your stuff there

scherler · 2018-11-07T12:37:48Z

core/src/main/resources/jenkins/model/Jenkins/login.jelly

              <div class="footer">
-
-                <st:include it="${simpleDecorator}" page="simple-footer.jelly" optional="true"/>
+                <j:forEach var="simpleDecorator" items="${simpleDecorators}">


This is actually the only change you need besides creating the methods to get all decorators.

scherler · 2018-11-07T12:38:35Z

core/src/main/java/hudson/Functions.java

     * @since 2.128
+     * @deprecated use {@link #getSimplePageDecorators()} instead
     */
+    @Deprecated


scherler · 2018-11-07T12:38:48Z

core/src/main/java/hudson/Functions.java

+     * Gets all {@link SimplePageDecorator}s for the login page.
+     * @since TODO
+     */
+    public static List<SimplePageDecorator> getSimplePageDecorators() {


scherler · 2018-11-07T12:38:59Z

core/src/main/java/jenkins/model/SimplePageDecorator.java

+     * Returns all login page decorators.
+     * @since TODO
+     */
+    public static List<SimplePageDecorator> all() {


scherler · 2018-11-07T12:39:14Z

core/src/main/resources/hudson/security/HudsonPrivateSecurityRealm/signup.jelly

    <!-- in case of error we want to surround the form elements with an error hint -->
    <j:set var="inputClass" value="${data.errorMessage!=null ? 'danger' : 'normal'}"/>
-    <j:set var="simpleDecorator" value="${h.simplePageDecorator}"/>
+    <j:set var="simpleDecorators" value="${h.simplePageDecorators}"/>


You need both

scherler · 2018-11-07T12:39:37Z

core/src/main/resources/hudson/security/HudsonPrivateSecurityRealm/signup.jelly

            <!-- we do not want bots on this page -->
            <meta name="ROBOTS" content="NOFOLLOW"/>
-            <!-- css styling, will fallback to default implementation -->
-            <st:include it="${simpleDecorator}" page="simple-head.jelly" optional="true"/>


scherler · 2018-11-07T12:40:06Z

core/src/main/resources/hudson/security/HudsonPrivateSecurityRealm/signup.jelly

                                }
                            </script>
                        </j:if>
+                        <div class="footer">


I am fine with that

scherler · 2018-11-07T12:40:18Z

core/src/main/resources/jenkins/model/Jenkins/login.jelly

  <!-- in case of error we want to surround the form elements with an error hint -->
  <j:set var="inputClass" value="${error ? 'danger' : 'normal'}"/>
-  <j:set var="simpleDecorator" value="${h.simplePageDecorator}"/>
+  <j:set var="simpleDecorators" value="${h.simplePageDecorators}"/>


see above, you need both

scherler · 2018-11-07T12:40:39Z

core/src/main/resources/jenkins/model/Jenkins/login.jelly

      <!-- we do not want bots on this page -->
      <meta name="ROBOTS" content="NOFOLLOW" />
      <!-- css styling, will fallback to default implementation -->
-      <st:include it="${simpleDecorator}" page="simple-head.jelly" optional="true"/>


scherler · 2018-11-07T12:40:47Z

core/src/main/resources/jenkins/model/Jenkins/login.jelly

        <j:otherwise>
          <div class="simple-page" role="main">
            <div class="modal login">
-              <st:include it="${simpleDecorator}" page="simple-header.jelly" optional="true"/>


jvz · 2018-11-07T18:06:41Z

Ah, I see what you mean. I'll update this to only add support for multiple footers. Then I can set a low priority on my decorator.

jvz · 2018-11-07T20:45:29Z

Redone in #3721.

[FIX JENKINS-54325] Add support for multiple SimplePageDecorators in …

3fd970c

…login.jelly Signed-off-by: Matt Sicker <boards@gmail.com>

daniel-beck requested a review from scherler October 30, 2018 20:31

Wadeck requested changes Oct 30, 2018

View reviewed changes

jvz added 3 commits October 30, 2018 16:10

Deprecate Functions.getSimplePageDecorator

89ccd6a

Signed-off-by: Matt Sicker <boards@gmail.com>

Add id to login password field

245f979

Signed-off-by: Matt Sicker <boards@gmail.com>

Add support for multiple SimplePageDecorators in signup.jelly

213ec32

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz commented Oct 30, 2018

View reviewed changes

Wadeck approved these changes Oct 30, 2018

View reviewed changes

scherler suggested changes Nov 5, 2018

View reviewed changes

scherler reviewed Nov 7, 2018

View reviewed changes

jvz mentioned this pull request Nov 7, 2018

[JENKINS-54325] Add support for multiple footers in SimplePageDecorator #3721

Merged

4 tasks

jvz closed this Nov 7, 2018

jvz deleted the simple-page-decorators branch November 7, 2018 20:45

Uh oh!

[FIX JENKINS-54325] Add support for multiple SimplePageDecorators in … #3710

[FIX JENKINS-54325] Add support for multiple SimplePageDecorators in … #3710

Uh oh!

Conversation

jvz commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changelog entries

Submitter checklist

Desired reviewers

Uh oh!

Wadeck left a comment

Choose a reason for hiding this comment

Uh oh!

jvz commented Oct 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wadeck left a comment

Choose a reason for hiding this comment

Uh oh!

scherler left a comment

Choose a reason for hiding this comment

Uh oh!

jvz commented Nov 5, 2018

Uh oh!

scherler commented Nov 5, 2018

Uh oh!

jvz commented Nov 5, 2018

Uh oh!

scherler commented Nov 5, 2018

Uh oh!

jvz commented Nov 5, 2018

Uh oh!

scherler commented Nov 5, 2018

Uh oh!

jvz commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scherler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvz commented Nov 7, 2018

Uh oh!

jvz commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jvz commented Oct 29, 2018 •

edited

Loading

jvz commented Nov 5, 2018 •

edited

Loading