Alexander Wels has posted comments on this change.

Change subject: webadmin: Added handling missing languages
......................................................................


Patch Set 1: Verified

(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>
Done
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>
Done
Line 76:   </dependencies>
Line 77: 
Line 78:   <build>
Line 79: 


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>
Done
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 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);
The problem the attribute might not have been set, and thus the result is null. 
You can't cast a null to a boolean.
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);
See my other comment, AFTER I have done this, the cast is fine, however the 
cast happens before the first time this is called.
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");
Of course, D'oh! I can't believe I missed that. Will fix that soon.
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());
Sure makes no difference to the compiler, I am sure it will optimize the extra 
variable away.
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>
Good point, will keep it generic here, and if needed add a more specific 
message for RHEV-M
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) {
Done
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.");
Done
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: 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

Reply via email to