Rewrote IndividualListController to handle error conditions from within FreeMarker. Reorganized controller control flow to account for interdependencies between body and title.

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/freemarker/AboutController.java

@@ -2,7 +2,9 @@
 
 package edu.cornell.mannlib.vitro.webapp.controller.freemarker; 
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import org.apache.commons.logging.Log;
@@ -24,6 +26,13 @@ public class AboutController extends FreeMarkerHttpServlet {
         body.put("aboutText", portal.getAboutText());
         body.put("acknowledgeText", portal.getAcknowledgeText()); 
         
+        // Test of #list directive in template on undefined, null, or empty values
+        // Basic idea: empty list okay, null or undefined value not okay
+        // List<String> apples = new ArrayList<String>();  // no error
+        // List<String> apples = null; // error
+        // body.put("apples", apples);
+        // no apples in body: error
+   
         String bodyTemplate = "about.ftl";             
         return mergeBodyToTemplate(bodyTemplate, body);
 

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/freemarker/BrowseController.java

@@ -18,12 +18,11 @@ import edu.cornell.mannlib.vitro.webapp.dao.filtering.filters.VitroFilterUtils;
 import edu.cornell.mannlib.vitro.webapp.dao.filtering.filters.VitroFilters;
 import edu.cornell.mannlib.vitro.webapp.flags.PortalFlag;
 import edu.cornell.mannlib.vitro.webapp.view.VClassGroupView;
+import freemarker.template.SimpleSequence;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import freemarker.template.*;
-
 import javax.servlet.ServletContext;
 
 import java.util.*;
@@ -69,7 +68,9 @@ public class BrowseController extends FreeMarkerHttpServlet {
 
     protected String getBody() {
 
-    	Map body = new HashMap();
+    	Map<String, Object> body = new HashMap<String, Object>();
+        String bodyTemplate = "classGroups.ftl"; 
+        String message = null;
         
     	// Set main page template attributes specific to this page
     	// But the template should control this! Try putting in a div inside the content.
@@ -80,22 +81,25 @@ public class BrowseController extends FreeMarkerHttpServlet {
 
     	//PortalFlag portalState= vreq.getPortalFlag();
 
-    	String message = "";
+    	List<VClassGroup> groups = getGroups(vreq.getWebappDaoFactory().getVClassGroupDao(), portalId);
-    	List<VClassGroup> groups = getGroups(vreq.getWebappDaoFactory().getVClassGroupDao(), vreq.getPortal().getPortalId());
     	if (groups == null || groups.isEmpty()) {
     		message = "There are not yet any items in the system.";
-    		body.put("message", message); 
     	}
     	else {
+    	    // FreeMarker will wrap vcgroups in a SimpleSequence. So do we want to create the SimpleSequence directly?
+    	    // But, makes code less portable to another system.
+    	    // SimpleSequence vcgroups = new SimpleSequence(groups.size());   	    
     	    List<VClassGroupView> vcgroups = new ArrayList<VClassGroupView>(groups.size());   	    
-    		Iterator<VClassGroup> i = groups.iterator();
+    		for (VClassGroup g: groups) {
-    		while (i.hasNext()) {
+    		    vcgroups.add(new VClassGroupView(g));
-    		    vcgroups.add(new VClassGroupView(i.next()));
     		}
     		body.put("classGroups", vcgroups);
     	} 
     	
-        String bodyTemplate = "classGroups.ftl";       
+    	if (message != null) {
+    	    body.put("message", message);
+    	} 
+    	
         return mergeBodyToTemplate(bodyTemplate, body);
     }
 

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/freemarker/FreeMarkerHttpServlet.java

@@ -66,17 +66,12 @@ public class FreeMarkerHttpServlet extends VitroHttpServlet {
 	protected String appName;
 	protected Map<String, Object> root = new HashMap<String, Object>();
     
-	// Some servlets have their own doGet() method, in which case they need to call 
-	// doSetup(), setTitle(), setBody(), and write() themselves. Other servlets define only
-	// a getBody() and getTitle() method and use the parent doGet() method.
     public void doGet( HttpServletRequest request, HttpServletResponse response )
 		throws IOException, ServletException {
 
     	try {
-    	    callSuperGet(request, response);  // ??
 	        doSetup(request, response);
-	        setTitle();	        
+	        setTitleAndBody();
-	        setBody();	        
 	        write(response);
        
 	    } catch (Throwable e) {
@@ -91,88 +86,21 @@ public class FreeMarkerHttpServlet extends VitroHttpServlet {
 		doGet(request, response);
 	}
 
-	protected void setBody() {
+	// Basic setup needed by all controllers
-	    root.put("body", getBody());
+    protected void doSetup(HttpServletRequest request, HttpServletResponse response) {
-	}
         
-	protected void setSharedVariable(String key, Object value) {
+        if ( !(this instanceof FreeMarkerComponentGenerator) ) {
-        try {
-            config.setSharedVariable(key, value);
-        } catch (TemplateModelException e) {
-            log.error("Can't set shared variable '" + key + "'.");
-        }	    
-	}
-	
-	protected void setTitle() {
-	    setSharedVariable("title", getTitle());	    
-	}
-
-    protected String getTitle() { 
-    	return null; 
-    }
-    
-    protected String getBody() {
-    	return null;
-    }
-    
-    protected StringWriter mergeToTemplate(String templateName, Map<String, Object> map) {
-    	
-        Template template = null;
-        try {
-        	template = config.getTemplate(templateName);
-        } catch (IOException e) {
-        	log.error("Cannot get template " + templateName);
-        }
-        StringWriter sw = new StringWriter();
-        if (template != null) {	        
-            try {
-				template.process(map, sw);
-			} catch (TemplateException e) {
-				// TODO Auto-generated catch block
-				e.printStackTrace();
-			} catch (IOException e) {
-				// TODO Auto-generated catch block
-				e.printStackTrace();
-			}        	
-        }
-        return sw;
-    }
-
-    protected String mergeBodyToTemplate(String templateName, Map<String, Object> map) {
-        templateName = "body/" + templateName;
-    	String body = mergeToTemplate(templateName, map).toString();
-    	return body;
-    }
-    
-    protected void write(HttpServletResponse response) {
-
-        String templateName = "page/default.ftl";
-        StringWriter sw = mergeToTemplate(templateName, root);          
-        try {
-            PrintWriter out = response.getWriter();
-            out.print(sw);     
-        } catch (IOException e) {
-            // TODO Auto-generated catch block
-            e.printStackTrace();
-        }                    
-    }
-    
-    protected void callSuperGet(HttpServletRequest request, HttpServletResponse response) {
             try {
                 super.doGet(request,response);   
             } catch (ServletException e) {
-            // TODO Auto-generated catch block
+                log.error("Servlet exception calling VitroHttpRequest.doGet()");
                 e.printStackTrace();
             } catch (IOException e) {
-            // TODO Auto-generated catch block
+                log.error("IO exception calling VitroHttpRequest.doGet()");
                 e.printStackTrace();
             }                
         }
         
-    // RY This needs to be broken out as is for FreeMarkerComponentGenerator, which should not
-    // include callSuperGet(). So it's only temporary.
-    protected void doSetup(HttpServletRequest request, HttpServletResponse response) {
- 
         vreq = new VitroRequest(request);
         this.response = response;
         portal = vreq.getPortal(); 
@@ -211,6 +139,34 @@ public class FreeMarkerHttpServlet extends VitroHttpServlet {
         setSharedVariable("scripts", new ScriptList()); 
     }
 
+    // Define template locations. Template loader will look first in the theme-specific
+    // location, then in the vitro location.
+    // RY We cannot do this in FreeMarkerSetup because (a) the theme depends on the portal,
+    // and we have multi-portal installations, and (b) we need to support theme-switching on the fly.
+    // To make more efficient, we could do this once, and then have a listener that does it again 
+    // when theme is switched. BUT this doesn't support (a), only (b), so  we have to do it on every request.
+    protected final void setTemplateLoader() {
+        
+        String themeTemplateDir = context.getRealPath(getThemeDir()) + "/ftl";
+        String vitroTemplateDir = context.getRealPath("/templates/freemarker");
+
+        try {
+            FileTemplateLoader themeFtl = new FileTemplateLoader(new File(themeTemplateDir));
+            FileTemplateLoader vitroFtl = new FileTemplateLoader(new File(vitroTemplateDir));
+            ClassTemplateLoader ctl = new ClassTemplateLoader(getClass(), "");
+            TemplateLoader[] loaders = new TemplateLoader[] { themeFtl, vitroFtl, ctl };
+            MultiTemplateLoader mtl = new MultiTemplateLoader(loaders);
+            config.setTemplateLoader(mtl);
+        } catch (IOException e) {
+            log.error("Error loading templates");
+        }
+        
+    }
+
+    private TabMenu getTabMenu() {
+        return new TabMenu(vreq, portalId);
+    }
+    
     public String getThemeDir() {
         return portal.getThemeDir().replaceAll("/$", "");
     }
@@ -307,32 +263,98 @@ public class FreeMarkerHttpServlet extends VitroHttpServlet {
 
     }
     
-    // Define template locations. Template loader will look first in the theme-specific
+    // Default case is to set title first, because it's used in the body. However, in some cases
-    // location, then in the vitro location.
+    // the title is based on values computed during compilation of the body (e.g., IndividualListController). 
-    // RY We cannot do this in FreeMarkerSetup because (a) the theme depends on the portal,
+    // Individual controllers can override this method to set title and body together. End result must be:
-    // and we have multi-portal installations, and (b) we need to support theme-switching on the fly.
+    // body is added to root with key "body" 
-    // To make more efficient, we could do this once, and then have a listener that does it again 
+    // title is set as a shared variable with key "title" 
-    // when theme is switched. BUT this doesn't support (a), only (b), so  we have to do it on every request.
+    // This can be achieved by making sure setBody() and setTitle() are called.
-	protected final void setTemplateLoader() {
+    protected void setTitleAndBody() {
+        setTitle();
+        setBody();
+    }
 
-	    String themeTemplateDir = context.getRealPath(getThemeDir()) + "/ftl";
+	protected void setBody() {
-	    String vitroTemplateDir = context.getRealPath("/templates/freemarker");
+	    root.put("body", getBody());
+	}
 
+    protected String getBody() {
+        return ""; // body should never be null
+    }
+    
+	protected void setTitle() {
+	    String title = getTitle();
+	    // If the individual controller fails to assign a non-null, non-empty title
+	    if (StringUtils.isEmpty(title)) {
+	        title = appName;
+	    }
+	    // Title is a shared variable because it's used in both body and head elements.
+	    setSharedVariable("title", title); 
+	}
+	
+    protected String getTitle() { 
+    	return "";
+    }
+
+    protected void setSharedVariable(String key, Object value) {
         try {
-            FileTemplateLoader themeFtl = new FileTemplateLoader(new File(themeTemplateDir));
+            config.setSharedVariable(key, value);
-            FileTemplateLoader vitroFtl = new FileTemplateLoader(new File(vitroTemplateDir));
+        } catch (TemplateModelException e) {
-            ClassTemplateLoader ctl = new ClassTemplateLoader(getClass(), "");
+            log.error("Can't set shared variable '" + key + "'.");
-            TemplateLoader[] loaders = new TemplateLoader[] { themeFtl, vitroFtl, ctl };
+        }       
-            MultiTemplateLoader mtl = new MultiTemplateLoader(loaders);
+    }
-            config.setTemplateLoader(mtl);
+    
+    protected StringWriter mergeToTemplate(String templateName, Map<String, Object> map) {
+    	
+        Template template = null;
+        try {
+        	template = config.getTemplate(templateName);
         } catch (IOException e) {
-            log.error("Error loading templates");
+        	log.error("Cannot get template " + templateName);
+        }
+        StringWriter sw = new StringWriter();
+        if (template != null) {	        
+            try {
+				template.process(map, sw);
+			} catch (TemplateException e) {
+				// TODO Auto-generated catch block
+				e.printStackTrace();
+			} catch (IOException e) {
+				// TODO Auto-generated catch block
+				e.printStackTrace();
+			}        	
+        }
+        return sw;
     }
 
+    protected String mergeBodyToTemplate(String templateName, Map<String, Object> map) {
+        templateName = "body/" + templateName;
+    	String body = mergeToTemplate(templateName, map).toString();
+    	return body;
     }
     
-	private TabMenu getTabMenu() {
+    protected void write(HttpServletResponse response) {
-	    return new TabMenu(vreq, portalId);
+
+        String templateName = "page/" + getPageTemplateName();
+        
+        StringWriter sw = mergeToTemplate(templateName, root);          
+        try {
+            PrintWriter out = response.getWriter();
+            out.print(sw);     
+        } catch (IOException e) {
+            log.error("FreeMarkerHttpServlet cannot write output");
+            e.printStackTrace();
+        }                    
+    }
+    
+    // Can be overridden by individual controllers
+    protected String getPageTemplateName() {
+        return "default.ftl";
+    }
+
+
+    public static boolean isConfigured() {
+        return config != null;
     }
     
     // TEMPORARY for transition from JSP to FreeMarker. Once transition
@@ -348,11 +370,7 @@ public class FreeMarkerHttpServlet extends VitroHttpServlet {
         request.setAttribute("ftl_footer", fcg.getFooter());       
     }
     
-    public static boolean isConfigured() {
+    /* ******************** Static utilities ******************* */
-        return config != null;
-    }
-    
-    /* ******************** Utilities ******************* */
 
     public static String getUrl(String path) {
         if ( ! path.startsWith("/") ) {

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/freemarker/IndividualListController.java

@@ -2,144 +2,119 @@
 
 package edu.cornell.mannlib.vitro.webapp.controller.freemarker;
 
-import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
-import javax.servlet.RequestDispatcher;
-import javax.servlet.ServletException;
-import javax.servlet.ServletOutputStream;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
 import edu.cornell.mannlib.vitro.webapp.beans.Individual;
 import edu.cornell.mannlib.vitro.webapp.beans.VClass;
 import edu.cornell.mannlib.vitro.webapp.beans.VClassGroup;
+import edu.cornell.mannlib.vitro.webapp.utils.StringUtils;
 import edu.cornell.mannlib.vitro.webapp.view.IndividualView;
 
+/** 
+ * Generates a list of individuals for display in a template 
+ */
 public class IndividualListController extends FreeMarkerHttpServlet {
   
-    long startTime = -1;
-    
     private static final long serialVersionUID = 1L;   
     private static final Log log = LogFactory.getLog(IndividualListController.class.getName());
     private VClass vclass = null;
+    private String title = null;
     
-    /**
+    protected void setTitleAndBody() {
-     * This generates a list of entities and then sends that
+        setBody();        
-     * off to a jsp to be displayed.
+    }
-     *
+
-     * Expected parameters:
+    protected String getBody() {
-     *
+ 
-     * Expected Attributes:
+        Map<String, Object> body = new HashMap<String, Object>();
-     * entity - set to entity to display properties for.
+        String bodyTemplate = "individualList.ftl";
-     *
+        String errorMessage = null;
-     * @author bdc34
+        String message = null;
-     */
         
-    // TODO Rewrite error cases to use FreeMarker templates. Restructure so we're always doing the body
-    // and then calling writeOutput().
-    public void doGet( HttpServletRequest req, HttpServletResponse res )
-    throws IOException, ServletException {
-        startTime = System.currentTimeMillis(); // TODO: remove
         try {
-            super.doSetup(req, res);
             Object obj = vreq.getAttribute("vclass");
             vclass = null;
             if ( obj == null ) { // look for vitroclass id parameter
-                String vitroClassIdStr=req.getParameter("vclassId");
+                String vitroClassIdStr = vreq.getParameter("vclassId");
-                if (vitroClassIdStr!=null && !vitroClassIdStr.equals("")) {
+                if ( !StringUtils.isEmpty(vitroClassIdStr)) { 
                     try {
                         //TODO have to change this so vclass's group and entity count are populated
                         vclass = vreq.getWebappDaoFactory().getVClassDao().getVClassByURI(vitroClassIdStr);
                         if (vclass == null) {
                             log.error("Couldn't retrieve vclass " + vitroClassIdStr);   
-                                response.sendRedirect(Routes.BROWSE + "?"+vreq.getQueryString());
+                            errorMessage = "Class " + vitroClassIdStr + " not found";
                         }
                     } catch (Exception ex) {
-                            throw new HelpException("IndividualListController: request parameter 'vclassId' must be a URI string");
+                        throw new HelpException("IndividualListController: request parameter 'vclassId' must be a URI string.");
                     }
                 }
             } else if (obj instanceof VClass) {
                 vclass = (VClass)obj;
             } else {
                 throw new HelpException("IndividualListController: attribute 'vclass' must be of type "
-                        + VClass.class.getName() );
+                        + VClass.class.getName() + ".");
             }
+            
             if (vclass != null) {
-                setBody();                
+                // Create list of individual view objects
-                write(response);
-            }
-        // RY Rewrite error cases for FreeMarker, not JSP
-        } catch (HelpException help){
-            doHelp(response);
-        } catch (Throwable e) {
-            vreq.setAttribute("javax.servlet.jsp.jspException",e);
-            RequestDispatcher rd = req.getRequestDispatcher("/error.jsp");
-            rd.forward(vreq, response);
-        }
-    }
-
-    public void doPost(HttpServletRequest request, HttpServletResponse response)
-    throws ServletException,IOException {
-        doGet(request, response);
-    }
-
-    protected String getBody() {
- 
-        Map<String, Object> body = new HashMap<String, Object>();
-        
-        // Create list of individuals
                 List<Individual> individualList = vreq.getWebappDaoFactory().getIndividualDao().getIndividualsByVClass(vclass);
                 List<IndividualView> individuals = new ArrayList<IndividualView>(individualList.size());
-        Iterator<Individual> i = individualList.iterator();
-        while (i.hasNext()) {
-            individuals.add(new IndividualView(i.next()));
-        }        
-        body.put("individuals", individuals);
 
-        // But the JSP version includes url rewriting via URLRewritingHttpServletResponse
+                if (individualList == null) {
-        // RY *** FIX  - define getUrl method of IndividualView
+                    // RY Is this really an error? 
-        body.put("individualUrl", getUrl("/entity?home=" + portalId + "&uri="));
-
-        if (individuals == null) {
                     log.error("individuals list is null");
+                    message = "No individuals to display.";
+                } else {            
+                    for (Individual i: individualList) {
+                        individuals.add(new IndividualView(i));
+                    }                   
                 }
 
-        // Use instead of getTitle(), because we have a subtitle too
+                // Set title and subtitle. Title will be retrieved later in getTitle().   
-        String title = "";
                 VClassGroup classGroup = vclass.getGroup();       
                 if (classGroup == null) {
                     title = vclass.getName();
                 } else {
                     title = classGroup.getPublicName();
-            setSharedVariable("subTitle", vclass.getName());
+                    body.put("subtitle", vclass.getName());
-        }
-        setSharedVariable("title", title);
-
-        String templateName = "individualList.ftl";
-        return mergeBodyToTemplate(templateName, body);
-
                 }
                 
-    // RY Rewrite as a template
+                body.put("individuals", individuals);
-    private void doHelp(HttpServletResponse res)
+            }   
-    throws IOException, ServletException {
+            
-        ServletOutputStream out = res.getOutputStream();
+        } catch (HelpException help){
-        res.setContentType("text/html; charset=UTF-8");
+            errorMessage = "Request attribute 'vclass' or request parameter 'vclassId' must be set before calling. Its value must be a class uri."; 
-        out.println("<html><body><h2>Quick Notes on using EntityList:</h2>");
+        } catch (Throwable e) {
-        out.println("<p>request.attributes 'entities' must be set by servlet before calling."
+            bodyTemplate = "error.ftl";
-                +" It must be a List of Entity objects </p>");
+        }
-        out.println("</body></html>");
+
+        if (errorMessage != null) {
+            bodyTemplate = "errorMessage.ftl";
+            body.put("errorMessage", errorMessage);
+        } else if (message != null) {
+            body.put("message", message);
+        }
+        
+        setTitle();
+    
+        return mergeBodyToTemplate(bodyTemplate, body);
+    }
+   
+    protected String getTitle() {
+        // The title is determined during compilation of the body, so we put it in an instance variable
+        // to be retrieved later.
+        return title;
     }
       
     private class HelpException extends Throwable {
+        private static final long serialVersionUID = 1L;
+
         public HelpException(String string) {
             super(string);
         }

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/freemarker/Routes.java

@@ -11,7 +11,7 @@ public class Routes {
     public static final String BROWSE = "/browse";
     public static final String COMMENT_FORM = "/comments";
     public static final String INDIVIDUAL = "/individual";
-    public static final String INDIVIDUAL_LIST = "/entitylist"; // "/individuallist";
+    public static final String INDIVIDUAL_LIST = "/individuallist"; // "/entitylist"; "/individuallist";
     public static final String SEARCH = "/search"; 
     public static final String TERMS_OF_USE = "/termsOfUse";
     

webapp/src/edu/cornell/mannlib/vitro/webapp/view/IndividualView.java

@@ -25,6 +25,8 @@ public class IndividualView extends ViewObject {
         return individual.getName();
     }
     
+    // RY However, the moniker should undergo p:process but the class name shouldn't! 
+    // So, it needs to be callable from Java.
     public String getTagline() {
         String tagline = individual.getMoniker();
         return StringUtils.isEmpty(tagline) ? individual.getVClass().getName() : tagline;

webapp/web/templates/freemarker/body/commentForm/form.ftl

@@ -1,6 +1,6 @@
 <#-- $This file is distributed under the terms of the license in /doc/license.txt$ -->
 
-<#-- Comment form -->
+<#-- Contact form -->
 
 <div class="staticPageBackground feedbackForm">
 

webapp/web/templates/freemarker/body/error.ftl

@@ -0,0 +1,7 @@
+<#-- $This file is distributed under the terms of the license in /doc/license.txt$ -->
+
+<#-- Template for general system error. -->
+
+<p>There was an error in the system.</p>
+
+<p>Return to the <a href="${urls.home}">home page</a>.</p> 

webapp/web/templates/freemarker/body/errorMessage.ftl

@@ -0,0 +1,7 @@
+<#-- $This file is distributed under the terms of the license in /doc/license.txt$ -->
+
+<#-- Standard template to display an error message generated from any controller. Keeps this out of individual templates. -->
+
+<#if errorMessage??>
+    <p>${errorMessage}</p>
+</#if>

webapp/web/templates/freemarker/body/individualList.ftl

@@ -7,9 +7,12 @@
     <div class="individualList">
         <h2>${title}</h2>
         <#if subtitle??>
-            <h4>${subTitle}"</h4>
+            <h4>${subtitle}</h4>
         </#if>
         
+        <#if message??>
+            <p>${message}</p>
+        <#else>
             <#-- RY NEED TO ACCOUNT FOR p:process stuff -->
             <ul>
                 <#list individuals as individual>
@@ -18,5 +21,7 @@
                     </li>
                 </#list>
             </ul>
+        </#if>
     </div>   
+
 </div>