CAS-1425: aligned view names to match the jsp filename #444
Conversation
|
What value is this adding? This means that deployers who previously overrode the file in their local overlay are now no longer overriding that file and will have to rename. |
|
I think this is an improvement to allow for matching file/view names, so that dynamic view resolvers can be implemented. What I really hope to see is that we remove the property file views, and have the resolver simply just work with the viewname, which is Spring's default. This way, it would make it completely unnecessary for one to include this file in the overlays. Filenames just have to match logical view names. As I mentioned, this would also one to create dynamic view resolvers that would show a different CAS login page, or use a different context path for view names that may be entirely separate, and not limited to just JS/CSS. |
|
The renaming is useful only if we go as far as @mmoayyed proposed: why not address that in this pull request? |
|
|
||
| viewServiceSsoErrorView.(class)=org.springframework.web.servlet.view.JstlView | ||
| viewServiceSsoErrorView.url=/WEB-INF/view/jsp/default/ui/serviceErrorSsoView.jsp | ||
|
|
||
| ### CAS statistics view | ||
| viewStatisticsView.(class)=org.springframework.web.servlet.view.JstlView |
There was a problem hiding this comment.
Why are we keep just this line?!
There was a problem hiding this comment.
Well, it's two lines... And the view is not technically part of the public facing ui. It's in monitoring and not default/ui
viewStatisticsView.(class)=org.springframework.web.servlet.view.JstlView
viewStatisticsView.url=/WEB-INF/view/jsp/monitoring/viewStatistics.jsp
Would you recommend it going somewhere else?
There was a problem hiding this comment.
I would really like for them to be gone, so whatever controller is routing to these views should similarly discover the JSP automatically.
|
John, I think we could also work on an alternative implementation of a resolver, that resolves a view name by the theme defined in the registry entry. Does that make sense? So for instance, if the theme is defined for a service to be "student", the resolver would look into /web-inf/view/jsp/student directory to find relevant views. This is something that is fairly on-demand. |
|
@jtgasper3 How are we doing here? Anything I can do to help? |
|
Should I include a commented out config for the alternate view resolver in the context file or just define it in documentation on the wiki? |
|
Docs would be better, I suppose. We do have a section on UI customization and theming, this would be a good addition to that block. |
| @@ -0,0 +1,24 @@ | |||
| ### Login view (/login) | |||
There was a problem hiding this comment.
Note the filename change...
There was a problem hiding this comment.
I am still struggling to understand why this file is needed. Wouldn't the controller be able to simply map the view name to the file name? Why have the view explicitly here?
|
Changes Unknown when pulling fea5751 on Unicon:CAS-1425 into * on Jasig:master*. |
| protected AbstractUrlBasedView buildView(final String viewName) throws Exception { | ||
| final RequestContext requestContext = RequestContextHolder.getRequestContext(); | ||
| final WebApplicationService service = (WebApplicationService) WebUtils.getService(requestContext); | ||
| final RegisteredService registeredService = this.servicesManager.findServiceBy(service); |
There was a problem hiding this comment.
You should make sure the service is not disabled.
There was a problem hiding this comment.
Is disabled service the same thing as a non-existing service entry? I'd argue that perhaps we should allow admins to apply a different theme/view set to a disabled entry e.g. "The scholarship app is down for repair".)
Either way it is a simple fix.
There was a problem hiding this comment.
No, a disabled service is registeredService.isEnabled(). Non-existing services are not allowed to use CAS anyways. The most interesting aspect of this pull is the ability to decorate the login page and I would like to stick to this. Not much else is going to be customized, but we can certainly track what you're describing in a separate issue.
There was a problem hiding this comment.
Checking for a disabled service will be in the next commit.
|
The proposed changes have been committed, with the exception of using the default theme if a service is disabled. It's coded, but I help on the commit until discussion is concluded. |
|
Hi guys, Is there anything else that can/should be done for this? |
| LOGGER.debug("Prefix {} set for service {} with theme {}", prefix, s 628C ervice, themeId); | ||
|
|
||
| //Set the prefix based on the retrieved themeId & resolve the view. | ||
| super.setPrefix(prefix); |
There was a problem hiding this comment.
I'm concerned about this: super.setPrefix(prefix);. What happens in a multi-threading environment when different services are called at the same time with different associated themes?
There was a problem hiding this comment.
I see where you are coming from. I have fixed that by pulling the buildView code from each of the base classes and incorporated it directly.
|
Changes Unknown when pulling 66643d2 on Unicon:CAS-1425 into * on Jasig:master*. |
| @@ -58,6 +59,8 @@ | |||
|
|
|||
| private String casTicketSuffix; | |||
|
|
|||
| private String viewPath = "/WEB-INF/view/jsp/monitoring/viewStatistics.jsp"; | |||
There was a problem hiding this comment.
sorry... why exactly are we hard-coding a path here?
There was a problem hiding this comment.
I wouldn't call it hard coded, but a realistic default. There is a setter that allows for the path of the view to be changed.
|
how did we go from aligning view names to a new theme resolver utilizing hard-coded paths? |
| @@ -158,14 +158,14 @@ | |||
| the "viewGenericLogin" is the end state for when a user attempts to login without coming directly from a service. | |||
| They have only initialized their single-sign on session. | |||
| --> | |||
| <end-state id="viewGenericLoginSuccess" view="casLoginGenericSuccessView" /> | |||
| <end-state id="viewGenericLoginSuccess" view="casGenericSuccess" /> | |||
There was a problem hiding this comment.
why are all these changing? what benefit are we getting from changing these right now?
There was a problem hiding this comment.
I think that the idea is to have the same view name as the JSP name. I doesn't change anything for the customized JSP pages, right?
There was a problem hiding this comment.
@leleuj, you are correct that this bring together a view's jsp filename with the viewname referenced in the login-webflow.xml. As for customized JSP pages, it shouldn't change anything if the filenames where kept the same.
(Although I've seen some implementors that give the jsp file a unique name and update the default_view.properties be commenting out the default and putting in a replacement. sigh)
|
Doesn't hard-coded paths mean that files can't be shared/leveraged across themes? |
|
+1 |
|
@battags, per you last comment:
|
|
Changes Unknown when pulling f690c06 on Unicon:CAS-1425 into * on Jasig:master*. |
|
So, where do we stand on this? |
Conflicts: cas-server-webapp/src/main/webapp/WEB-INF/cas-servlet.xml
|
I merged this PR with master once to bring it up to speed, and also updated docs and comments inside the class |
|
Bump. Everything looks good? Plan to merge by Friday? |
|
Team, planning to merge. Please let me know if you see a blocker with this issue. |
CAS-1425: aligned view names to match the jsp filename
Some view names in default_views.properties have been updated to match the file names of the jsp's. The respective view names in the login-webflow.xml have been updated as well. This pull fixes https://github.com/Jasig/cas/issues/490