[JENKINS-39300] View.getDisplayName() is ignored when rendering tabs #2603

stephenc · 2016-10-27T10:47:53Z

Also:

adds support for descriptor visibility filtering on views
fixes l10n of AllView
adds support for contextual create permission of specific view types
some other small code fixes spotted while touching the files

@jenkinsci/code-reviewers @reviewbybees

…ew tabs - Also fix the stapler documentation for the <l:tab> tag

stephenc · 2016-10-27T10:52:00Z

core/src/main/java/hudson/model/View.java

+     * Returns the {@link ViewDescriptor} instances that can be instantiated for the {@link ViewGroup} in the current
+     * {@link StaplerRequest}. <strong>NOTE: This method must be called from a {@link StaplerRequest}</strong>
+     * @return the list of instantiable {@link ViewDescriptor} instances for the current {@link StaplerRequest}
+     */


Note: this is the javadoc that applied to the method before my changes as the method would have blown up with a NPE if invoked outside of the newView.jelly screen of a ViewGroup as soon as AllView.isInstantiable() was called

Perhaps just check owner == null and return all()?

Need to also check whether the StaplerRequest is null.

- Also while we are at it, modernise ViewDescriptor instantiation to allow for descriptor visibility filtering

…es of views

…criptorVisiblilityFilters too

ghost · 2016-10-27T11:32:09Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

…siblilityFilters

daniel-beck · 2016-10-27T13:02:23Z

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

@@ -3052,7 +3052,7 @@ public void run(Reactor session) throws Exception {
                // initialize views by inserting the default view if necessary
                // this is both for clean Jenkins and for backward compatibility.
                if(views.size()==0 || primaryView==null) {
-                    View v = new AllView(Messages.Hudson_ViewName());
+                    View v = new AllView("all");


This looks like it fixes JENKINS-38606.

It is. But will it cause a fallout in upgrading Jenkins instances?

Will only affect new instances. Old instances can get the fix by recreating the All view (add new view, change default, delete all view, add back all view, change default, tidy up)

We could be more aggressive and rename the AllView to all but it is valid to give it whatever name you want... though perhaps we need some further tweaks

This looks like a reasonable default for new All views. No need to edit existing ones. Would just cause confusion and broken links.

daniel-beck · 2016-10-27T13:04:11Z

core/src/main/resources/lib/layout/tab.jelly

+      Whether the tab is active or not
+    </st:attribute>
+    <st:attribute name="title">
+      The title of the tab


Do we have an @since equivalent for Jelly tags?

I am restoring behaviour that was broken in this earlier commit (appears to be removed by omission as the old style documentation of parameters was retained)

jenkinsci/stapler#84 created for you ;-)

…ly if safe to fix - Don't fix if there is already a view with the new default name - Don't fix if the AllView is not the primary view

stephenc · 2016-10-28T08:44:52Z

@daniel-beck @oleg-nenashev so I have added a fix for JENKINS-38606... and we need to be able to invoke that fix from plugins (e.g. folders plugin) so I cannot make that @Restricted before you ask!

daniel-beck · 2016-10-28T08:50:41Z

have added a fix for JENKINS-38606

Not actually safe, as there are code paths that add the view name to the URL even if it's the default view.

I don't think we should fix JENKINS-38606 for existing installations. It modifies user data for a pretty weak reason.

stephenc · 2016-10-28T08:59:11Z

@daniel-beck OK, I have made this opt-in. I still think we should have the ability for an admin to force the fix applied everywhere as it could be quite difficult to fix all the AllViews scattered everywhere otherwise

daniel-beck · 2016-10-28T10:42:59Z

Is this really not trivial to take care of in a script console script with maybe five lines?

Do we really need a startup parameter (to be added in service configurations) for a one-time cleanup action here?

stephenc · 2016-10-28T13:02:26Z

@daniel-beck really? show me the five lines that find every ViewGroup (including the User properties for all users) and the folders etc and does the fix-up? For most admins it should not be an issue... but if it is an issue it could be very painful to fix by script or manually. Much better to provide a hook to fix the issue that is available to everyone IMHO.

stephenc · 2016-10-29T21:11:09Z

@daniel-beck can you respond please? Would like to get the change at least merged before we hit one of the last likely candidates for the next LTS

daniel-beck · 2016-10-30T00:14:28Z

I still have my doubt but I'd be happy to be outvoted. So far, nobody else has cared enough though @jenkinsci/code-reviewers Ping!). FWIW I would like to see other opinions on solving this using a new parameter.

rsandell · 2016-11-01T11:38:06Z

🐝

jglick

Some minor inline comments, and tests are failing; 👍 on concept.

jglick · 2016-11-01T17:07:07Z

core/src/main/java/hudson/model/AllView.java

+     * @since FIXME
+     */
+    @Nonnull
+    public static String applyJenkins38606Fixup(@Nonnull List<View> views, @Nonnull String primaryView) {


@Restricted(NoExternalUse.class)

(and remove @since) unless this is something you really expect plugins to use.

I expect plugins that implement ViewGroup will need to call into this method

Is there a proposed PR calling it? From cloudbees-folder? Then you should probably have a clearer name.

jglick · 2016-11-01T17:07:33Z

core/src/main/java/hudson/model/AllView.java

+     * fixing the view name by setting the system property {@code hudson.mode.AllView.JENKINS-38606} to {@code true}.
+     * Use this method to round-trip the primary view name, e.g.
+     * {@code primaryView = applyJenkins38606Fixup(views, primaryView)}
+     * NOTE: we can only fix the localized name of an {@link AllView} if it is the primary view as otherwise urls


jglick · 2016-11-01T17:08:11Z

core/src/main/java/hudson/model/AllView.java

+     * {@code primaryView = applyJenkins38606Fixup(views, primaryView)}
+     * NOTE: we can only fix the localized name of an {@link AllView} if it is the primary view as otherwise urls
+     * would change, whereas the primary view is special and does not normally get accessed by the
+     * {@code /view/_name_} url.


…though it can be, and those URLs will break. Right?

Yes, and there are links in Jenkins to the default view's /view/whatever URL.

Yes, actually the new item link has switched from /view/All/newJob to /view/all/newJob (note the lowercase in the default view name).

jglick · 2016-11-01T17:09:32Z

core/src/main/java/hudson/model/AllView.java

+            // modern name, we are safe
+            return primaryView;
+        }
+        if (SystemProperties.getBoolean(AllView.class.getName()+".JENKINS-38606", false)) {


getBoolean(String) suffices.

jglick · 2016-11-01T17:10:57Z

core/src/main/java/hudson/model/AllView.java

+                    // name conflict, we cannot rename the all view anyway
+                    return primaryView;
+                }
+                if (StringUtils.equals(v.getViewName(), primaryView)) {


AFAICT getViewName() should be marked @Nonnull.

meaning you could use

if (v.getViewName().equals(primaryView)) {

jglick · 2016-11-01T17:18:45Z

core/src/main/java/hudson/model/ViewDescriptor.java

+     * @return the validation result.
+     * @since FIXME
+     */
+    @SuppressWarnings("unused") // expose utility check method to subclasses


Is there a particular subclass you had in mind? Not in this PR; some other linked one?

Yes but it needs to use the Referer header hack to resolve the View so probably waiting on dedicated support in stapler for that

So where is that subclass?

jglick · 2016-11-01T17:19:20Z

core/src/main/java/hudson/model/ViewDescriptor.java

+    /**
+     * Returns true if this {@link View} type is applicable in the specific {@link ViewGroup}.
+     *
+     * @return true to indicate applicable, in which case the view will be instantiable within the given owner.


Specify default implementation.

jglick · 2016-11-01T17:19:28Z

core/src/main/java/hudson/model/ViewDescriptor.java

+    /**
+     * Returns true if this {@link View} type is applicable to the given {@link ViewGroup} type.
+     *
+     * @return true to indicate applicable, in which case the view will be instantiable within the type of owner.


Specify default implementation.

jglick · 2016-11-01T17:23:10Z

core/src/main/resources/hudson/model/MyViewsProperty/newView.jelly

@@ -32,7 +32,7 @@ THE SOFTWARE.
    <st:include page="sidepanel.jelly" it="${it.user}"/>

    <l:main-panel>
-      <j:invokeStatic var="views" className="hudson.model.View" method="allInstantiable" />
+      <j:invokeStatic var="views" className="hudson.model.View" method="allInstantiable"/>


Gratuitous diff hunk.

jglick · 2016-11-01T17:23:38Z

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

@@ -31,7 +31,7 @@ THE SOFTWARE.
    <st:include page="sidepanel.jelly" />

    <l:main-panel>
-      <j:invokeStatic var="views" className="hudson.model.View" method="allInstantiable" />
+      <j:invokeStatic var="views" className="hudson.model.View" method="allInstantiable"/>


stephenc

@jglick I think i have addressed all your comments (in some way or other)

stephenc · 2016-11-03T21:56:37Z

core/src/main/java/hudson/model/AllView.java

+                    // name conflict, we cannot rename the all view anyway
+                    return primaryView;
+                }
+                if (StringUtils.equals(v.getViewName(), primaryView)) {


stephenc · 2016-11-03T21:57:43Z

core/src/main/java/hudson/model/ViewDescriptor.java

+     * @return the validation result.
+     * @since FIXME
+     */
+    @SuppressWarnings("unused") // expose utility check method to subclasses


Yes but it needs to use the Referer header hack to resolve the View so probably waiting on dedicated support in stapler for that

jglick

Would like to discuss having better URL compatibility, and would like to see the downstream PR that various API methods here imply.

jglick · 2016-11-22T19:36:03Z

core/src/main/java/hudson/model/AllView.java

+     * @since FIXME
+     */
+    @Nonnull
+    public static String applyJenkins38606Fixup(@Nonnull List<View> views, @Nonnull String primaryView) {


Is there a proposed PR calling it? From cloudbees-folder? Then you should probably have a clearer name.

jglick · 2016-11-22T19:36:35Z

core/src/main/java/hudson/model/AllView.java

+                    // name conflict, we cannot rename the all view anyway
+                    return primaryView;
+                }
+                if (StringUtils.equals(v.getViewName(), primaryView)) {


meaning you could use

if (v.getViewName().equals(primaryView)) {

jglick · 2016-11-22T19:37:55Z

core/src/main/java/hudson/model/ViewDescriptor.java

+     * @return the validation result.
+     * @since FIXME
+     */
+    @SuppressWarnings("unused") // expose utility check method to subclasses


So where is that subclass?

jglick · 2016-11-22T19:39:26Z

core/src/main/java/hudson/views/ListViewColumn.java

-                if (!lvc.shownByDefault())      continue; // skip this
+                if (!lvc.shownByDefault()) {
+                    continue; // skip this
+                }


gratuitous reformatting

jglick · 2016-11-22T19:43:08Z

core/src/main/java/hudson/model/AllView.java

+            // modern name, we are safe
+            return primaryView;
+        }
+        if (SystemProperties.getBoolean(AllView.class.getName()+".JENKINS-38606")) {


Making this disabled by default makes it less than useful.

IIUC the concern is about compatibility for direct links to, say, http://jenkins/view/All/job/something/. But that is easily solved: amend ViewGroupMixIn.getView (called from Jenkins.getView(String), the Stapler URL binding) to fall back to searching by View.getDisplayName if no match is found via getViewName.

Actually no, so the concern here is that you had been using say the DE locale when you created the folder... thus you would have http://jenkins/jobs/MyFolder/view/Alle/job/something, the less likely concern is if you had started Jenkins in the IT locale (before changing it to something else) and you had http://jenkins/view/Tutto/job/something

Now what will happen with the fix-up is that it will spot these mistakes and say "Oh, those are localized names, that should really be all"

Now when a user with the LT locale configured for their browser clicks on either of those two links:

http://jenkins/jobs/MyFolder/view/Alle/job/something, well the primary AllView's displayName in LT is Visi so falling back to View.getDisplayName() will still not match

http://jenkins/view/Tutto/job/something again has the same issue.

But thanks to pointing out ViewGroupMixIn.getView what happens if we do not find a matching view name?

jenkins/core/src/main/java/hudson/model/ViewGroupMixIn.java

Lines 97 to 102 in 9172bca

if (name != null && !name.equals(primaryView())) {

// Fallback to subview of primary view if it is a ViewGroup

View pv = getPrimaryView();

if (pv instanceof ViewGroup)

return ((ViewGroup)pv).getView(name);

}

So what will happen is, the name is not null and the name is not all and there was no other view with the name Visi... so we will display the primary view.

So actually I think we can default this to ON and stop worrying

Damn I read that wrong... but I think it does give us a solution instead

…r migration - This means we can default the migration to enabled

stephenc · 2016-12-07T10:09:27Z

See jenkinsci/cloudbees-folder-plugin#79 for example usage of migrateLegacyPrimaryAllViewLocalizedName method

stephenc · 2016-12-07T10:11:24Z

@rsandell @daniel-beck @jglick PR has been updated

jglick

Sounds good to the extent I understand it.

jglick · 2016-12-08T15:22:29Z

core/src/main/java/hudson/model/ViewGroupMixIn.java

@@ -99,6 +101,15 @@ public View getView(String name) {
            View pv = getPrimaryView();
            if (pv instanceof ViewGroup)
                return ((ViewGroup)pv).getView(name);
+            if (pv instanceof AllView && AllView.DEFAULT_VIEW_NAME.equals(pv.name)) {


Sounds right but hard for me at least to follow how this will work in progress; recommend having some code coverage using @LocalData to prove it works.

jglick · 2016-12-08T15:22:40Z

core/src/main/java/hudson/security/ACL.java

+     * @param c the container of the item.
+     * @param d the descriptor of the view to be created.
+     * @throws AccessDeniedException if the user doesn't have the permission.
+     * @since 1.607


jglick

Ah but lots of test failures.

daniel-beck · 2016-12-10T12:55:27Z

One test still fails:

Expected: a string containing "ERROR: Jenkins does not allow to delete 'All' view"
     but: was "
ERROR: Jenkins does not allow to delete 'all' view
"

daniel-beck · 2016-12-10T16:42:44Z

core/src/main/java/hudson/model/ViewGroupMixIn.java

+            if (pv instanceof AllView && AllView.DEFAULT_VIEW_NAME.equals(pv.name)) {
+                // JENKINS-38606: primary view is the default AllView, is somebody using an old link to localized form?
+                for (Locale l : Locale.getAvailableLocales()) {
+                    if (name.equals(Messages._Hudson_ViewName().toString(l))) {


So changing the translations will break this compatibility measure for that language? Is this so rare to be irrelevant?

stephenc · 2016-12-11T23:44:42Z

I think we're good to merge now... i'll hold off until tomorrow and then merge just in case anyone has and issues with the test changes

Noting #2668, #2664, #2663, #2662, #2661, #2667, #2666, #2652, #2643, #2591, #2658, #2645, #2660, #2603

jglick · 2017-10-15T22:41:21Z

core/src/main/java/hudson/model/AllView.java

+                        // bingo JENKINS-38606 detected
+                        LOGGER.log(Level.INFO,
+                                "JENKINS-38606 detected for AllView in {0}; renaming view from {1} to {2}",
+                                new Object[]{allView.owner.getUrl(), DEFAULT_VIEW_NAME});


stephenc added 2 commits October 27, 2016 11:05

[FIXED JENKINS-39300] Use View.getDisplayName() when rendering the vi…

0763ffd

…ew tabs - Also fix the stapler documentation for the <l:tab> tag

[JENKINS-39300] Add displayName validation API to ViewDescriptor

5b1ace0

stephenc commented Oct 27, 2016

View reviewed changes

stephenc added 3 commits October 27, 2016 11:57

[JENKINS-39300 Follow-up] Fix the AllView tab title to reflect l10n

7c43986

- Also while we are at it, modernise ViewDescriptor instantiation to allow for descriptor visibility filtering

[JENKINS-39300 Follow-up] Allow create permission restriction for typ…

056b3b0

…es of views

Redundant null check removed

e0a80dc

stephenc force-pushed the jenkins-39300 branch from b3f71bb to e0a80dc Compare October 27, 2016 10:57

[JENKINS-39300 Follow-up] JobFilters and Columns should work with Des…

417e0d8

…criptorVisiblilityFilters too

[JENKINS-39300 Follow-up] Default columns should respect DescriptorVi…

f3d13af

…siblilityFilters

daniel-beck reviewed Oct 27, 2016

View reviewed changes

[FIXED JENKINS-38606] Detect and fix localized AllView name if and on…

8cb1070

…ly if safe to fix - Don't fix if there is already a view with the new default name - Don't fix if the AllView is not the primary view

stephenc added 2 commits October 28, 2016 09:52

[JENKINS-38606] Typos

19e8be4

[JENKINS-38606] Require the user to Opt-in to the fix

559a5af

oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Oct 29, 2016

[JENKINS-39300] Sort the views by displayName

beaabb0

recampbell assigned jglick Nov 1, 2016

jglick requested changes Nov 1, 2016

View reviewed changes

[JENKINS-39300] Address review comments

5106675

stephenc commented Nov 3, 2016

View reviewed changes

jglick approved these changes Nov 22, 2016

View reviewed changes

[JENKINS-38606] Allow legacy localized links to continue working afte…

fd2009a

…r migration - This means we can default the migration to enabled

stephenc mentioned this pull request Dec 7, 2016

[WIP JENKINS-38606 Follow-up] Ensure AllView gets migrated jenkinsci/cloudbees-folder-plugin#79

Merged

jglick approved these changes Dec 8, 2016

View reviewed changes

jglick requested changes Dec 8, 2016

View reviewed changes

[JENKINS-39300] Update tests

d1c40f4

daniel-beck reviewed Dec 10, 2016

View reviewed changes

[JENKINS-39300] Fix final test case

3538931

stephenc merged commit e18b34c into jenkinsci:master Dec 12, 2016

stephenc deleted the jenkins-39300 branch December 12, 2016 15:13

oleg-nenashev added a commit that referenced this pull request Dec 16, 2016

Changelog: updates towards 2.37

1c1e9d5

Noting #2668, #2664, #2663, #2662, #2661, #2667, #2666, #2652, #2643, #2591, #2658, #2645, #2660, #2603

jglick mentioned this pull request Jan 3, 2017

Make compilable with JDK 9 javac #2697

Merged

daniel-beck mentioned this pull request Aug 5, 2017

Minor fixes to the title of a view #2969

Merged

jglick mentioned this pull request Oct 15, 2017

[JENKINS-47416] Do not call ViewGroup.getUrl from migrateLegacyPrimaryAllViewLocalizedName #3088

Merged

jglick reviewed Oct 15, 2017

View reviewed changes

	if (name != null && !name.equals(primaryView())) {
	// Fallback to subview of primary view if it is a ViewGroup
	View pv = getPrimaryView();
	if (pv instanceof ViewGroup)
	return ((ViewGroup)pv).getView(name);
	}

[JENKINS-39300] View.getDisplayName() is ignored when rendering tabs #2603

[JENKINS-39300] View.getDisplayName() is ignored when rendering tabs #2603

Conversation

stephenc commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Oct 28, 2016

daniel-beck commented Oct 28, 2016

stephenc commented Oct 28, 2016

daniel-beck commented Oct 28, 2016

stephenc commented Oct 28, 2016

stephenc commented Oct 29, 2016

daniel-beck commented Oct 30, 2016

rsandell commented Nov 1, 2016

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Dec 7, 2016

stephenc commented Dec 7, 2016

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

daniel-beck commented Dec 10, 2016

daniel-beck Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Dec 11, 2016

Choose a reason for hiding this comment

stephenc Oct 28, 2016 •

edited

Loading

daniel-beck Dec 10, 2016 •

edited

Loading