[GitHub] [jspwiki] juanpablo-santos commented on a change in pull request #44: JSPWIKI-1117

[GitBox <gi...@apache.org>]

juanpablo-santos commented on a change in pull request #44:
URL: https://github.com/apache/jspwiki/pull/44#discussion_r585068939



##########
File path: jspwiki-main/src/main/java/org/apache/wiki/preferences/Preferences.java
##########
@@ -125,19 +133,21 @@ public static void reloadPreferences( final PageContext pageContext ) {
      * @param request
      * @param prefs The default hashmap of preferences
      */
-	private static void parseJSONPreferences( final HttpServletRequest request, final Preferences prefs ) {
-        final String prefVal = TextUtil.urlDecodeUTF8( HttpUtil.retrieveCookieValue( request, "JSPWikiUserPrefs" ) );
-        if( prefVal != null ) {
+	private static void parseJSONPreferences( final Preferences prefs, final String prefVal ) {       

Review comment:
       it would be nice if this method is made package-private, so some unit tests defining its behaviour can be made. The alternative would be unit testing the `reloadPreferences` method, but the first approach seems easier.

##########
File path: jspwiki-war/src/main/webapp/templates/default/commonheader.jsp
##########
@@ -156,4 +177,76 @@ String.I18N.PREFIX = "javascript.";
          src="<wiki:Link format='url' templatefile='skins/' /><c:out value='${prefs.SkinName}/skin.js' />" ></script>
 </c:if>
 
+
+
+<%-- Support for cookie acceptance --%>
+ <c:if test='${!(prefs.cookies)}'>
+<script type="text/javascript">
+document.addEventListener("DOMContentLoaded", readyForCookieTest);
+var appBaseName = document.querySelector('meta[name="wikiApplicationName"]').content;
+
+function sendAjaxCookieConfirmation(dialogElem){
+		var request = new XMLHttpRequest();
+		var appBaseUrl = "/"+appBaseName+"/";
+		var cookieAcceptParam = "?cookies=true";
+		request.open('GET', appBaseUrl+cookieAcceptParam, true);
+
+		request.onload = function() {
+		  if (this.status >= 200 && this.status < 400) {
+			// Success!
+			var resp = this.response;
+			console.log("Cookies accepted!");
+			dialogElem.toggle();
+			dialogElem.hide();
+			dialogElem.destroy();
+		  } else {
+			// We reached our target server, but it returned an error
+		  }
+	};
+
+	request.onerror = function() {
+	  // There was a connection error of some sort
+	};	
+		request.send();
+	}
+var cookiesAllow = ${prefs.cookies};
+
+function setCookie(cname, cvalue, exdays) {
+	  var d = new Date();
+	  // Cookie setting lasts for 1 day before deleted from browser.
+	  // TODO: Setup in server preference for client's preferred cookie lifetime.
+	  d.setTime(d.getTime() + (exdays*24*60*60*1000));
+	  var expires = "expires="+ d.toUTCString();
+		document.cookie ="CookiePrefs={"+ cname + ":'" + cvalue + "'}";
+	}
+
+function readyForCookieTest(){
+if(!cookiesAllow){	
+		var cookieDialogHtml = "<div class='dialog-message'>This application uses cookies to work properly. By using the application you understand and accept the use of necessary cookies. Click OK to proceed.</div>";

Review comment:
       Both texts should be i18nized, see [line 90 before](https://github.com/apache/jspwiki/pull/44/files#diff-813bef24eb53b1d4b32778c239d73a4f760fcedccaf84799870302f7c3abef3fR90). Other than that (and the tests before), the PR looks good to me. @brushed WDYT?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org