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-32178] Fix broken links to branches from custom views #35

Merged
merged 1 commit into from May 25, 2016

Conversation

@mjdetullio
Copy link
Contributor

commented Apr 25, 2016

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

@stephenc

This comment has been minimized.

Copy link
Member

commented May 3, 2016

Hmmm I need to think about this one

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2016

Custom views are essentially broken, solely due to the override. Any old links referencing jenkins/my-multi-branch-project/branch/feature-abc/ will still work since getBranch(String) is not being removed.

@stephenc

This comment has been minimized.

Copy link
Member

commented May 4, 2016

wouldn't a better option be to add a hidden action called job. As views delegate to the folder for actions that would resolve the job action and then that can just be a StaplerProxy for the corresponding call to getBranch

@mjdetullio mjdetullio force-pushed the mjdetullio:JENKINS-32178 branch from 17fdc13 to 5473a55 May 6, 2016

@mjdetullio mjdetullio changed the title [JENKINS-32178] Remove getUrlChildPrefix() override. [JENKINS-32178] Fix broken links to branches from custom views May 6, 2016

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2016

I took another crack at it using an action as you suggested, completely replacing the previous implementation. The action is completely hidden as you suggest and will work with any override for MultiBranchProject#getUrlChildPrefix() by delegating to MultiBranchProject#getItem(String)

The only problem I could find with this is shown in this screenshot, where an extra blank breadcrumb is added to the top of the page.

image

@mjdetullio mjdetullio force-pushed the mjdetullio:JENKINS-32178 branch 4 times, most recently from eb51de0 to 6bd779b May 6, 2016

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2016

Also I looked into using StaplerProxy, but by the time the action is handled by Stapler, branch is no longer part of the remaining URL. StaplerProxy can't delegate individual methods to another object, only the remaining part of the URL to another object, so I wasn't able to use it. A normal public method sufficed.

@mjdetullio mjdetullio force-pushed the mjdetullio:JENKINS-32178 branch from 6bd779b to 8e57691 May 6, 2016

@stephenc

This comment has been minimized.

Copy link
Member

commented May 7, 2016

I think that is fixable. At the very least we could turn it into a temporary redirect to the correct URL since we just want to fix users browsing by clicking links so they should not have links to sub-elements. But I think there is another way that removes the breadcrumb also

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2016

Well, the effect of the extra breadcrumb is visual only. It's not clickable and does not generate children. The children are already correctly listed by the view breadcrumb.

I looked at the breadcrumbBar.jelly file and it does not appear there is a way to exclude the entry. h#isModel will always return true for actions and I'm not aware of any way to negate the second comparison with the ancestor URL. https://github.com/jenkinsci/jenkins/blob/jenkins-2.1/core/src/main/resources/lib/layout/breadcrumbBar.jelly#L60

I don't think an HTTP redirect for the user is the correct approach. It could make things unnecessarily complex or broken for the JSON/XML web APIs.

@jglick

This comment has been minimized.

Copy link
Member

commented May 20, 2016

Why not just get rid of the custom URL prefix override? Would fix everything and the only patch is deleting a few lines of code.

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

That was the original patch but there was some debate from @stephenc

@jglick

This comment has been minimized.

Copy link
Member

commented May 20, 2016

I see. (Well, I do not see, since I guess you force-pushed a different commit, but I can imagine.) So @stephenc why do we care about preserving the custom URL prefix? Seems like it is much more of a hassle than it is worth.

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

Correct, I did force push over it and seemed to have lost the commit to the sands of time. The original change was removing the prefix override and deprecating MultiBranchProject#getBranch().

@stephenc

This comment has been minimized.

Copy link
Member

commented May 20, 2016

Because I would prefer to keep the semantic meaning. @jglick Ping me on Monday and we can discuss more

@jglick

This comment has been minimized.

Copy link
Member

commented May 20, 2016

In fact the current URL pattern breaks this too, which is causing me problems in jenkinsci/workflow-cps-plugin#12.

Note that OrganizationFolder just uses job/. I think after #8 we should just say that all ComputedFolders should behave as much like Folders as possible.

@jglick

This comment has been minimized.

Copy link
Member

commented May 23, 2016

@jglick

This comment has been minimized.

Copy link
Member

commented May 23, 2016

Could hardcode /(?:job|branch)/ as a regexp separator for that case but seems even uglier than it already is.

@stephenc

This comment has been minimized.

Copy link
Member

commented May 23, 2016

@jglick ok you won me over... reluctantly, but I'll accept us moving to job (though it would be much better if everyone could just move to item

@mjdetullio mjdetullio force-pushed the mjdetullio:JENKINS-32178 branch 2 times, most recently from 624d1ab to 97fa7fb May 24, 2016

[JENKINS-32178] Remove URL child prefix override.
By removing the prefix override, links to the sub-projects
on custom views will use the default "job", allowing the
views' existing Stapler URL bindings to correctly load the
page for the item.

@mjdetullio mjdetullio force-pushed the mjdetullio:JENKINS-32178 branch from 97fa7fb to 02e6a00 May 24, 2016

@mjdetullio

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2016

Updated PR back to original change

@jglick

This comment has been minimized.

Copy link
Member

commented May 24, 2016

👍

@stephenc stephenc merged commit c478564 into jenkinsci:master May 25, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.