Juan Hernandez has posted comments on this change.
Change subject: webadmin: Added handling missing languages
......................................................................
Patch Set 1: (10 inline comments)
....................................................
File backend/manager/modules/root/pom.xml
Line 65: <groupId>javax.servlet</groupId>
Line 66: <artifactId>jstl</artifactId>
Line 67: <version>1.2</version>
Line 68: <scope>provided</scope>
Line 69: </dependency>
We tend to use jboss artifacts for JEE apis. If possible add something like
this to the properties section of the root POM:
<javax.jstl.api.version>1.0.3.Final</javax.jstl.api.version>
This to the dependencies management section to the root POM:
<groupId>org.jboss.spec.javax.servlet.jstl</groupId>
<artifactId>jboss-jstl-api_1.2_spec</artifactId>
<version>${javax.jstl.api.version}</version>
<scope>provided</scope>
And then, in this POM just this:
<groupId>org.jboss.spec.javax.servlet.jstl</groupId>
<artifactId>jboss-jstl-api_1.2_spec</artifactId>
Line 70: <dependency>
Line 71: <groupId>org.mockito</groupId>
Line 72: <artifactId>mockito-all</artifactId>
Line 73: <version>1.9.5</version>
Line 71: <groupId>org.mockito</groupId>
Line 72: <artifactId>mockito-all</artifactId>
Line 73: <version>1.9.5</version>
Line 74: <scope>test</scope>
Line 75: </dependency>
Mockito is already included in the root POM, but not in the dependencies
management section (it should probably be). However you can use
${mockito.version} instead of 1.9.5.
Line 76: </dependencies>
Line 77:
Line 78: <build>
Line 79:
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/DocsServlet.java
Line 1: /**
Line 2: *
Line 3: */
Remove?
Line 4: package org.ovirt.engine.core;
Line 5:
Line 6: import java.io.File;
Line 7: import java.io.IOException;
Line 45: file = checkForIndex(request, response, file,
request.getPathInfo());
Line 46: if(null == file) {
Line 47: response.sendError(HttpServletResponse.SC_NOT_FOUND);
Line 48: } else if(!response.isCommitted()){ //If the response is
committed, we have already redirected.
Line 49: Object languagePageShown =
request.getSession(true).getAttribute(LANG_PAGE_SHOWN);
Isn't it safe to do here this?
Boolean languagePageShown = (Boolean) request....
Line 50: if(!file.equals(originalFile)) {
Line 51: //We determined that we are going to redirect the user
to the english version URI.
Line 52: String redirect = request.getServletPath() +
replaceLocaleWithUSLocale(request.getPathInfo(), locale);
Line 53: if((null == languagePageShown ||
!Boolean.parseBoolean(languagePageShown.toString()))) {
Line 50: if(!file.equals(originalFile)) {
Line 51: //We determined that we are going to redirect the user
to the english version URI.
Line 52: String redirect = request.getServletPath() +
replaceLocaleWithUSLocale(request.getPathInfo(), locale);
Line 53: if((null == languagePageShown ||
!Boolean.parseBoolean(languagePageShown.toString()))) {
Line 54:
request.getSession(true).setAttribute(LANG_PAGE_SHOWN, true);
As you are putting here always a boolean it should be safe to add the cast to
Boolean that I mention in the previous comment.
Line 55: request.setAttribute(LOCALE, locale);
Line 56: request.setAttribute(ENGLISH_HREF, redirect);
Line 57: RequestDispatcher dispatcher =
getServletContext().getRequestDispatcher("/help/no_lang.jsp");
Line 58: if (dispatcher != null) {
Line 53: if((null == languagePageShown ||
!Boolean.parseBoolean(languagePageShown.toString()))) {
Line 54:
request.getSession(true).setAttribute(LANG_PAGE_SHOWN, true);
Line 55: request.setAttribute(LOCALE, locale);
Line 56: request.setAttribute(ENGLISH_HREF, redirect);
Line 57: RequestDispatcher dispatcher =
getServletContext().getRequestDispatcher("/help/no_lang.jsp");
Can we put this JSP inside WEB-INF so that the user can't request it directly?
Line 58: if (dispatcher != null) {
Line 59: dispatcher.include(request, response);
Line 60: }
Line 61: } else {
Line 87:
Line 88: private String replaceLocaleWithUSLocale(String originalString,
Locale locale) {
Line 89: //Have to check against both the _ and - versions of the
locale to replace, as they might not be consistent.
Line 90: return originalString.replaceAll("/"+
locale.toLanguageTag().replaceAll("-", "\\\\-") + "|/" + locale.toString(),
Line 91: "/" + Locale.US.toLanguageTag());
I would appreciate if you can split this in several lines with some temporary
variables to make it more readable.
Line 92: }
Line 93:
Line 94: /**
Line 95: * Determines the locale based on the request passed in. It will
first try to determine the locale
....................................................
File backend/manager/modules/root/src/main/webapp/help/no_lang.jsp
Line 20: <div class="titlepage">
Line 21: <div>
Line 22: <p>It appears that you do not have the
<c:out value="${requestScope['locale'].getDisplayLanguage()}" /> language pack
installed.
Line 23: Please have your administrator install
the proper language by running:</p>
Line 24: <pre class="screen"><code
class="command">yum install rhevm-doc-<c:out
value="${requestScope['locale'].toLanguageTag()}" /></code></pre>
Upstream there is no rhevm-doc package, and I am not sure if we do have such a
thing. I would rather be generic and omit the "by running ..." part, or just
saying "by installing the appropriate package.
Line 25: <p>
Line 26: Please click <a
href="${requestScope['englishHref']}">here</a> for the English documentation.
This message will
Line 27: only be displayed once per session.
Line 28: </p>
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ServletUtils.java
Line 153: // All checks passed, the path is sane:
Line 154: return true;
Line 155: }
Line 156:
Line 157: public static File getFileFromString(String path, File base) {
This name is not very self explaining, maybe makeFileFromSanePath, or something
similar. My concern is that it is not clear that this is checking that the path
is sane.
Line 158: File file = null;
Line 159:
Line 160: if (path == null) {
Line 161: file = base;
Line 160: if (path == null) {
Line 161: file = base;
Line 162: }
Line 163: else if (!ServletUtils.isSane(path)) {
Line 164: log.error("The path \"" + path + "\" is not sane, will
send a 404 error response.");
I would change this log message to "... will return null" as the decission to
send the 404 error message is no longer here.
Line 165: }
Line 166: else {
Line 167: file = new File(base, path);
Line 168: }
--
To view, visit http://gerrit.ovirt.org/10065
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37156061cbdd7d2df3290c88dee933c41e0087c5
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
