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.

Sign up for GitHub

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

Jump to bottom

Issue 13192 edit mode cleanup #13193

Merged
merged 49 commits into from
Dec 27, 2017
Merged

Issue 13192 edit mode cleanup #13193

merged 49 commits into from
Dec 27, 2017

Conversation

wezell
Copy link
Contributor

@wezell wezell commented Dec 8, 2017 •
edited
Loading

This replaced the nasty code we have everywhere

boolean live = true;
boolean ADMIN_MODE = (session.getAttribute(com.dotmarketing.util.WebKeys.ADMIN_MODE_SESSION) != null);
boolean PREVIEW_MODE = ((session.getAttribute(com.dotmarketing.util.WebKeys.PREVIEW_MODE_SESSION) != null) && ADMIN_MODE);
boolean EDIT_MODE = ((session.getAttribute(com.dotmarketing.util.WebKeys.EDIT_MODE_SESSION) != null) && ADMIN_MODE);
if (EDIT_MODE || PREVIEW_MODE) {
	live = false;
}

with

PageMode mode = PageMode.get(request);
PageMode mode = PageMode.get(session);

mode.isAdmin ; 
mode.showLive ; 
mode.respectAnonPerms ;

if( PageMode.get(request).isAdmin){ doAdminStuff(); }

contentAPI.find("sad", user, mode.respectAnonPerms);

contentAPI.findByIdentifier("id", 1, mode.showLive, user, mode.respectAnonPerms);

PageMode.setPageMode(request, PageMode.PREVIEW_MODE);

etc....

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/util/WebKeys.java
@@ -258,9 +258,14 @@
public static final String ADMIN_MODE_COOKIE = "ADMIN_MODE_COOKIE";

// SESSION ATTRIBUTES
/*
public static final String EDIT_MODE_SESSION = "com.dotmarketing.EDIT_MODE_SESSION";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR This block of commented-out lines of code should be removed. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/viewtools/PersonaTool.java
@@ -41,8 +42,7 @@ public void init(Object obj) {

if (session != null) {
timemachine = session.getAttribute("tm_date") != null;
ADMIN_MODE = !timemachine && session != null
&& (session.getAttribute(com.dotmarketing.util.WebKeys.ADMIN_MODE_SESSION) != null);
boolean ADMIN_MODE = PageMode.get(request) .isAdmin;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Rename "ADMIN_MODE" which hides the field declared at line 33. rule
MAJOR Remove this useless assignment to local variable "ADMIN_MODE". rule
MINOR Remove this unused "ADMIN_MODE" local variable. rule
MINOR Rename this local variable to match the regular expression '^[a-z][a-zA-Z0-9]*$'. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/util/PageMode.java Outdated

public static PageMode get(HttpSession ses) {

PageMode mode = (ses != null && ses.getAttribute(com.dotmarketing.util.WebKeys.PAGE_MODE_SESSION) != null
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Immediately return this expression instead of assigning it to the temporary variable "mode". rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/servlets/ShortyServlet.java





Optional<ShortyId> shortOpt = APILocator.getShortyAPI().getShorty(id);
User user = WebAPILocator.getUserWebAPI().getLoggedInFrontendUser(request);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Remove this useless assignment to local variable "user". rule
MINOR Remove this unused "user" local variable. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/util/VelocityUtil.java Outdated

PageMode mode = (EDIT_MODE)
? PageMode.EDIT
: (PREVIEW_MODE)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Extract this nested ternary operation into an independent statement. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/util/VelocityUtil.java Outdated
? PageMode.EDIT
: (PREVIEW_MODE)
? PageMode.PREVIEW
: (ADMIN_MODE)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Extract this nested ternary operation into an independent statement. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rest/elasticsearch/ESContentResourcePortlet.java
@@ -117,21 +113,15 @@ public Response searchRaw(@Context HttpServletRequest request) {

HttpSession session = request.getSession();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Remove this useless assignment to local variable "session". rule
MINOR Remove this unused "session" local variable. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/viewtools/ESContentTool.java Outdated
import java.util.List;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Remove this unused import 'javax.servlet.http.HttpSession'. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/viewtools/content/ContentTool.java


PageMode mode = PageMode.get(req);
EDIT_OR_PREVIEW_MODE=!mode.showLive;
if(session!=null){
tmDate = (String) session.getAttribute("tm_date");
boolean tm=tmDate!=null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Remove this useless assignment to local variable "tm". rule
MINOR Remove this unused "tm" local variable. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/util/ConvertToPOJOUtil.java Outdated
final Class classToUse) throws ParseException, IllegalAccessException,
InvocationTargetException, InstantiationException, NoSuchMethodException, NoSuchFieldException {
public static <T> List<T> convertDotConnectMapToPOJO(List<Map<String, Object>> results, final Class classToUse)
throws IllegalAccessException, InstantiationException,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Remove the declaration of thrown exception 'java.lang.IllegalAccessException', as it cannot be thrown from method's body. rule
MINOR Remove the declaration of thrown exception 'java.lang.InstantiationException', as it cannot be thrown from method's body. rule

dev-dotcms
dev-dotcms reviewed Dec 8, 2017
View reviewed changes
dotCMS/src/main/java/com/dotmarketing/util/ConvertToPOJOUtil.java Outdated
InvocationTargetException, InstantiationException, NoSuchMethodException, NoSuchFieldException {
public static <T> List<T> convertDotConnectMapToPOJO(List<Map<String, Object>> results, final Class classToUse)
throws IllegalAccessException, InstantiationException,
NoSuchMethodException, NoSuchFieldException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Remove the declaration of thrown exception 'java.lang.NoSuchMethodException', as it cannot be thrown from method's body. rule
MINOR Remove the declaration of thrown exception 'java.lang.NoSuchFieldException', as it cannot be thrown from method's body. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/servlet/VelocityPreviewServlet.java
hostVariablesTemplate.merge(context, out);
template.merge(context, out);

} catch (ParseErrorException e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Either log or rethrow this exception. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/directive/FixBreaks.java
@@ -30,8 +30,6 @@ public String getName() {
return "fixBreaks";
}



final public boolean render(InternalContextAdapter context, Writer writer, Node node)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Reorder the modifiers to comply with the Java Language Specification. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/directive/FixBreaks.java
@@ -30,8 +30,6 @@ public String getName() {
return "fixBreaks";
}



final public boolean render(InternalContextAdapter context, Writer writer, Node node)
throws IOException, ResourceNotFoundException, ParseErrorException, MethodInvocationException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Remove the declaration of thrown exception 'org.apache.velocity.exception.ResourceNotFoundException' which is a runtime exception. rule
MINOR Remove the declaration of thrown exception 'org.apache.velocity.exception.ParseErrorException' which is a runtime exception. rule
MINOR Remove the declaration of thrown exception 'org.apache.velocity.exception.MethodInvocationException' which is a runtime exception. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java

try {
backendUser = com.liferay.portal.util.PortalUtil.getUser(request);
// Skin skin = backendUser.getSkin();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR This block of commented-out lines of code should be removed. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
* @throws DotDataException
* @throws DotSecurityException
*/
public static IHTMLPage getPage(Identifier id, HttpServletRequest request, boolean live, Context context) throws DotDataException, DotSecurityException{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Split this 153 characters long line (which is greater than 150 authorized). rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
context.put("dotPageMode", mode);
Date moddate = htmlPage.getModDate();

moddate = new Date(moddate.getTime());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Use the Java 8 Date and Time API instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java

//get the context from the request if possible
ChainedContext context;
if ( request.getAttribute( com.dotcms.rendering.velocity.Constants.VELOCITY_CONTEXT ) != null && request.getAttribute( com.dotcms.rendering.velocity.Constants.VELOCITY_CONTEXT ) instanceof ChainedContext ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Split this 215 characters long line (which is greater than 150 authorized). rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
* @throws DotSecurityException
* @throws DotDataException
*/
public static String getPageCacheKey(HttpServletRequest request, HttpServletResponse response) throws DotDataException, DotSecurityException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Reduce the number of returns of this method 6, down to the maximum allowed 3. rule
MAJOR Remove this unused method parameter "response". rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
return (ChainedContext) request.getAttribute( "velocityContext" );
} else {
RequestWrapper rw = new RequestWrapper( request );
if ( request.getAttribute( "User-Agent" ) != null && request.getAttribute( "User-Agent" ).equals( Constants.USER_AGENT_DOTCMS_BROWSER ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Split this 151 characters long line (which is greater than 150 authorized). rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
+ " and deleted=" + com.dotmarketing.db.DbConnectionFactory.getDBFalse() + "and language_id = "
+ (String) request.getSession().getAttribute(com.dotmarketing.util.WebKeys.HTMLPAGE_LANGUAGE) + ")", "UTF-8");
context.put("view", view);
} catch (Exception e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Catch a list of specific exception subtypes instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
public class VelocityUtil {

private static VelocityEngine ve = null;
private static boolean DEFAULT_PAGE_TO_DEFAULT_LANGUAGE = LanguageWebAPI.canDefaultPageToDefaultLanguage();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Rename this field "DEFAULT_PAGE_TO_DEFAULT_LANGUAGE" to match the regular expression '^[a-z][a-zA-Z0-9]*$'. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
private static VelocityEngine ve = null;
private static boolean DEFAULT_PAGE_TO_DEFAULT_LANGUAGE = LanguageWebAPI.canDefaultPageToDefaultLanguage();

private synchronized static void init(){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Reorder the modifiers to comply with the Java Language Specification. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
private static boolean DEFAULT_PAGE_TO_DEFAULT_LANGUAGE = LanguageWebAPI.canDefaultPageToDefaultLanguage();

private synchronized static void init(){
if(ve != null)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Missing curly brace. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
}
// nocache passed either as a session var, as a request var or as a
// request attribute
if ("no".equals(request.getParameter("dotcache"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL Define a constant instead of duplicating this literal "dotcache" 3 times. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
ve.init(SystemProperties.getProperties());

Logger.debug(VelocityUtil.class, SystemProperties.getProperties().toString());
}catch (Exception e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Catch a list of specific exception subtypes instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
* if we have a toolbox manager, get a toolbox from it See
* /WEB-INF/toolbox.xml
*/
context.setToolbox(getToolboxManager().getToolboxContext(context));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Remove this use of "setToolbox"; it is deprecated. rule
MINOR Remove this use of "getToolboxContext"; it is deprecated. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
// put the list of languages on the page
context.put("languages", getLanguages());
HttpSession session = request.getSession(false);
if(!UtilMethods.isSet(request.getAttribute(WebKeys.HTMLPAGE_LANGUAGE)) && session!=null)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Missing curly brace. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
HttpSession session = request.getSession(false);
if(!UtilMethods.isSet(request.getAttribute(WebKeys.HTMLPAGE_LANGUAGE)) && session!=null)
context.put("language", (String) session.getAttribute(com.dotmarketing.util.WebKeys.HTMLPAGE_LANGUAGE));
else
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Missing curly brace. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
backendUser = com.liferay.portal.util.PortalUtil.getUser(request);
// Skin skin = backendUser.getSkin();
// context.put("USER_SKIN", skin.getSkinId());
} catch (Exception nsue) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Catch a list of specific exception subtypes instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
velocityRootPath = Config.getStringProperty("VELOCITY_ROOT", "/WEB-INF/velocity");
if (velocityRootPath.startsWith("/WEB-INF")) {
Logger.debug(VelocityUtil.class, "Velocity ROOT Path not found, defaulting it to '/WEB-INF/velocity'");
String startPath = velocityRootPath.substring(0, 8);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Assign this magic number 8 to a well-named constant, and use the constant instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
Host host;
host = WebAPILocator.getHostWebAPI().getCurrentHost(request);
context.put("host", host);
} catch (Exception e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Catch a list of specific exception subtypes instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
if (velocityRootPath.startsWith("/WEB-INF")) {
Logger.debug(VelocityUtil.class, "Velocity ROOT Path not found, defaulting it to '/WEB-INF/velocity'");
String startPath = velocityRootPath.substring(0, 8);
String endPath = velocityRootPath.substring(9, velocityRootPath.length());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Assign this magic number 9 to a well-named constant, and use the constant instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java

// to check user has permission to publish this page
boolean permission = APILocator.getPermissionAPI().doesUserHavePermission(htmlPage, PERMISSION_PUBLISH, backendUser);
context.put("permission", new Boolean(permission));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Remove this "Boolean" constructor rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java


String adminRoleKey = "";
try {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Extract this nested try block into a separate method. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
Visitor visitor = (Visitor) request.getSession().getAttribute(WebKeys.VISITOR);
context.put("visitor", visitor);

} catch (Exception nsue) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Catch a list of specific exception subtypes instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java
try {
Role adminRole = APILocator.getRoleAPI().loadRoleByKey(Config.getStringProperty("CMS_ADMINISTRATOR_ROLE", "CMS Administrator"));
adminRoleKey = adminRole.getRoleKey();
} catch (Exception e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Either log or rethrow this exception. rule
MAJOR Either remove or fill this block of code. rule
MINOR Catch a list of specific exception subtypes instead. rule

dev-dotcms
dev-dotcms reviewed Dec 27, 2017
View reviewed changes
dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java


public String parseVelocity(String velocityCode, Context ctx){
VelocityEngine ve = VelocityUtil.getEngine();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Rename "ve" which hides the field declared at line 60. rule

@jgambarios jgambarios merged commit 76d9671 into master Dec 27, 2017
@jgambarios jgambarios deleted the issue-13192-edit-mode-cleanup branch December 27, 2017 17:00
@jgambarios jgambarios mentioned this pull request Dec 27, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdotcms jdotcms jdotcms left review comments

@dev-dotcms dev-dotcms dev-dotcms left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wezell @dev-dotcms @jdotcms @jgambarios @fmontes