From 9d4b521de79896d60f18636c0a5511187b48137a Mon Sep 17 00:00:00 2001 From: Charles Severance Date: Tue, 18 Jul 2017 13:52:08 -0400 Subject: [PATCH] SAK-30656 - Add delete tool checkboxes to delete deployment screen (#4617) --- .../org/sakaiproject/lti/api/LTIService.java | 2 + .../org/sakaiproject/util/foorm/Foorm.java | 4 + .../util/foorm/TestFoormJUnit.java | 30 ++++++- .../sakaiproject/lti/impl/BaseLTIService.java | 78 +++++++++++++++++-- .../sakaiproject/lti/impl/DBLTIService.java | 27 +++++-- .../src/bundle/ltitool.properties | 2 + .../sakaiproject/blti/tool/LTIAdminTool.java | 72 +++++++++-------- .../src/webapp/vm/lti_deploy_delete.vm | 17 ++++ 8 files changed, 183 insertions(+), 49 deletions(-) diff --git a/basiclti/basiclti-api/src/java/org/sakaiproject/lti/api/LTIService.java b/basiclti/basiclti-api/src/java/org/sakaiproject/lti/api/LTIService.java index bd8dbed7fd29..81dc819518f1 100644 --- a/basiclti/basiclti-api/src/java/org/sakaiproject/lti/api/LTIService.java +++ b/basiclti/basiclti-api/src/java/org/sakaiproject/lti/api/LTIService.java @@ -323,6 +323,8 @@ public interface LTIService { boolean deleteTool(Long key, String siteId); + public List deleteToolAndContents(Long key, String siteId); + boolean deleteToolDao(Long key, String siteId, boolean isAdminRole, boolean isMaintainRole); Map getTool(Long key, String siteId); diff --git a/basiclti/basiclti-common/src/java/org/sakaiproject/util/foorm/Foorm.java b/basiclti/basiclti-common/src/java/org/sakaiproject/util/foorm/Foorm.java index 745e7b8d4d44..f17e17edad6d 100644 --- a/basiclti/basiclti-common/src/java/org/sakaiproject/util/foorm/Foorm.java +++ b/basiclti/basiclti-common/src/java/org/sakaiproject/util/foorm/Foorm.java @@ -1460,6 +1460,10 @@ public String searchCheck(String search, String tableName, String[] fieldinfo) { if (search == null) { return null; } + //check if is a direct search + if (StringUtils.isNotEmpty(search) && search.matches("(\\w+\\.)?\\w+\\s*=.+")) { + return search; + } StringBuilder sb = new StringBuilder(); List tokens = getSearchTokens(search); List separators = getSearchSeparators(search); diff --git a/basiclti/basiclti-common/src/test/org/sakaiproject/util/foorm/TestFoormJUnit.java b/basiclti/basiclti-common/src/test/org/sakaiproject/util/foorm/TestFoormJUnit.java index 5a884b1d3820..edca85c3f5b5 100644 --- a/basiclti/basiclti-common/src/test/org/sakaiproject/util/foorm/TestFoormJUnit.java +++ b/basiclti/basiclti-common/src/test/org/sakaiproject/util/foorm/TestFoormJUnit.java @@ -117,8 +117,34 @@ public void testBasicFormExtraction() { @Test public void testSearchCheckSql() { Foorm foorm = new Foorm(); - // There was a problem with a NPE being thrown when there weren't any colons in the search - assertEquals(null, foorm.searchCheck("COLUMN = 'value'", "table", new String[]{})); + // SAK-32704 - Want to review all this can perhaps adjust how the decision is made + // about which search approach is in use + String whereStyle = "COLUMN = 'value'"; + assertEquals(whereStyle, foorm.searchCheck(whereStyle, "table", new String[]{})); + whereStyle = "COLUMN='value'"; + assertEquals(whereStyle, foorm.searchCheck(whereStyle, "table", new String[]{})); + whereStyle = "COLUMN='value' OR OTHER=42"; + assertEquals(whereStyle,foorm.searchCheck(whereStyle, "table", new String[]{})); + whereStyle = "COLUMN='value' OR ( OTHER=42 and zap=21)"; + assertEquals(whereStyle, foorm.searchCheck(whereStyle, "table", new String[]{})); + + // Workaround for the Null ones below - not pretty but works + whereStyle = "1=1 and (table.COLUMN LIKE '%zap%' OR ( othertable.OTHER=42 and zap=21))"; + assertEquals(whereStyle, foorm.searchCheck(whereStyle, "table", new String[]{})); + + // At some point we might want these to pass + whereStyle = "(1=1) and (table.COLUMN LIKE '%zap%' OR ( othertable.OTHER=42 and zap=21))"; + assertNull(foorm.searchCheck(whereStyle, "table", new String[]{})); + whereStyle = "table.COLUMN LIKE '%zap%' OR ( othertable.OTHER=42 and zap=21)"; + assertNull(foorm.searchCheck(whereStyle, "table", new String[]{})); + + // A parenthesis first should be OK + whereStyle = "(COLUMN='value') OR ( OTHER=42 and zap=21)"; + assertNull(foorm.searchCheck(whereStyle, "table", new String[]{})); + whereStyle = "( COLUMN = 'value' ) OR ( OTHER=42 and zap=21)"; + assertNull(foorm.searchCheck(whereStyle, "table", new String[]{})); + whereStyle = " ( COLUMN = 'value' ) OR ( OTHER=42 and zap=21)"; + assertNull(foorm.searchCheck(whereStyle, "table", new String[]{})); } @Test diff --git a/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/BaseLTIService.java b/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/BaseLTIService.java index efa969060867..0503fb1ae722 100644 --- a/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/BaseLTIService.java +++ b/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/BaseLTIService.java @@ -445,6 +445,73 @@ public boolean deleteTool(Long key, String siteId) { return deleteToolDao(key, siteId, isAdmin(siteId), isMaintain(siteId)); } + /** Delete a tool and delete the content items and site links assocated with the tool + * + * This is called by a maintain user in a regualr site and deletes a tool, its content + * item, and pages with it on the page. + * + * For the admin user in the !admin site - it deletes a tool and then removes the + * placements + pages from all the sites that have the tool - might take a second or two. + * + * @return A list of strings that are error messages + */ + @Override + public List deleteToolAndContents(Long key, String siteId) { + + List retval = new ArrayList (); + String errstr; + + List> contents; + if ( isAdmin(siteId) ) { + contents = this.getContentsDao("lti_content.tool_id = "+key,null,0,5000, null, isAdmin(siteId)); + } else { + contents = this.getContents("lti_content.tool_id = "+key, null,0,5000, siteId); + } + + for ( Map content : contents ) { + // the content with same tool id remove the content link first + Long content_key = foorm.getLongNull(content.get(LTIService.LTI_ID)); + if ( content_key == null ) continue; + + // Check the tool_id - just double checking in case the WHER clause fails + Long tool_id = foorm.getLongNull(content.get(LTIService.LTI_TOOL_ID)); + if ( ! key.equals(tool_id) ) continue; + + // Admin edits all sites with the content item + String contentSite = siteId; + if ( isAdmin(siteId) ) { + contentSite = content.get(LTIService.LTI_SITE_ID).toString(); + } + + // Is there is a tool placement in the left Nav (i.e. not Lessons) + // remove the tool content link page from the site + String pstr = (String) content.get(LTIService.LTI_PLACEMENT); + if ( pstr != null && pstr.length() > 1 ) { + errstr = this.deleteContentLink(content_key, contentSite); + if ( errstr != null ) { + M_log.debug(errstr); + retval.add(errstr); + } + } + + // remove the content item that depends on the tool + if ( ! this.deleteContent(content_key, contentSite) ) { + errstr = "Unable to delete content itemkey="+key+" site="+siteId; + M_log.debug(errstr); + retval.add(errstr); + } + } + + // We are going to delete the tool even if there were problems along the way + // Since that is the one thing we are supposed to do in this method + if ( ! this.deleteTool(key, siteId) ) { + errstr = "Unable to delete tool key="+key+" site="+siteId; + M_log.debug(errstr); + retval.add(errstr); + } + return retval; + } + @Override public Map getTool(Long key, String siteId) { return getToolDao(key, siteId, isAdmin(siteId)); @@ -656,14 +723,14 @@ protected Object insertToolSiteLinkDao(String id, String button_text, String sit catch (PermissionException ee) { retval = new String("0" + rb.getFormattedMessage("error.link.placement.update", new Object[]{id})); - M_log.warn(this + " cannot add page and basic lti tool to site " + siteId); + M_log.warn("Cannot add page and basic lti tool to site " + siteId); } } catch (IdUnusedException e) { // cannot find site retval = new String("0" + rb.getFormattedMessage("error.link.placement.update", new Object[]{id})); - M_log.warn(this + " cannot find site " + contentSite); + M_log.warn("Cannot find site " + contentSite); } return retval; @@ -700,7 +767,7 @@ protected String deleteContentLinkDao(Long key, String siteId, boolean isAdminRo } String siteStr = (String) content.get(LTI_SITE_ID); - // only admin can remve content from other site + // only admin can remove content from other site if ( ! siteId.equals(siteStr) && !isAdminRole ) { return rb.getString("error.placement.not.found"); } @@ -719,10 +786,11 @@ protected String deleteContentLinkDao(Long key, String siteId, boolean isAdminRo return rb.getString("error.placement.not.removed"); } } else { - M_log.warn(this + " LTI content="+key+" placement="+tool.getId()+" could not find page in site=" + siteStr); + M_log.warn("LTI content="+key+" placement="+tool.getId()+" could not find page in site=" + siteStr); } // Remove the placement from the content item + // Our caller can remove the contentitem if they like Properties newProps = new Properties(); newProps.setProperty(LTIService.LTI_PLACEMENT, ""); Object retval = updateContentDao(key, newProps, siteId, isAdminRole, isMaintainRole); @@ -736,7 +804,7 @@ protected String deleteContentLinkDao(Long key, String siteId, boolean isAdminRo } catch (IdUnusedException ee) { - M_log.warn(this + " LTI content="+key+" placement="+tool.getId()+" could not remove page from site=" + siteStr); + M_log.warn("LTI content="+key+" placement="+tool.getId()+" could not remove page from site=" + siteStr); return new String(rb.getFormattedMessage("error.link.placement.update", new Object[]{key.toString()})); } } diff --git a/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/DBLTIService.java b/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/DBLTIService.java index 554f1ff4d596..8e53bb7e5467 100644 --- a/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/DBLTIService.java +++ b/basiclti/basiclti-impl/src/java/org/sakaiproject/lti/impl/DBLTIService.java @@ -415,22 +415,29 @@ public Object updateContentDao(Long key, Object newProps, String siteId, key, newProps, siteId, isAdminRole, isMaintainRole); } + /** + * Get the contents for a search, add some data from site properties, and the launch + * from lti_tools - the dependency means that this will not find content items that do + * not have an associated tool. + */ public List> getContentsDao(String search, String order, int first, int last, String siteId, boolean isAdminRole) { + // It is important that any tables/columns added for the purposes of display or searching be + // LEFT JOIN - *any* INNER JOIN will function as a WHERE clause and will hide content String concatSearch = ("mysql".equals(m_sql.getVendor())) ? "CONCAT_WS('', lti_content.launch, lti_tools.launch)" : "(lti_content.launch || lti_tools.launch)"; String extraSelect = "SAKAI_SITE.TITLE AS SITE_TITLE, ssp1.VALUE AS SITE_CONTACT_NAME, ssp2.VALUE AS SITE_CONTACT_EMAIL, lti_tools.launch as URL, "+concatSearch+" AS searchURL"; - String joinClause = "JOIN SAKAI_SITE ON lti_content.SITE_ID = SAKAI_SITE.SITE_ID" + String joinClause = "LEFT JOIN SAKAI_SITE ON lti_content.SITE_ID = SAKAI_SITE.SITE_ID" + " LEFT JOIN SAKAI_SITE_PROPERTY ssp1 ON (lti_content.SITE_ID = ssp1.SITE_ID AND ssp1.name = 'contact-name')" + " LEFT JOIN SAKAI_SITE_PROPERTY ssp2 ON (lti_content.SITE_ID = ssp2.SITE_ID AND ssp2.name = 'contact-email')" - + " JOIN lti_tools ON (lti_content.tool_id = lti_tools.id)"; - + + " LEFT JOIN lti_tools ON (lti_content.tool_id = lti_tools.id)"; + String propertyKey = serverConfigurationService.getString(LTI_SITE_ATTRIBUTION_PROPERTY_KEY, LTI_SITE_ATTRIBUTION_PROPERTY_KEY_DEFAULT); if (StringUtils.isNotEmpty(propertyKey)) { extraSelect += ", ssp3.VALUE as ATTRIBUTION"; joinClause = joinClause + " LEFT JOIN SAKAI_SITE_PROPERTY ssp3 ON (lti_content.SITE_ID = ssp3.SITE_ID AND ssp3.name = '" + propertyKey + "')"; } - + final String[] fields = (String[])ArrayUtils.addAll(LTIService.CONTENT_MODEL, LTIService.CONTENT_EXTRA_FIELDS); if (order != null) { order = foorm.orderCheck(order, "lti_content", fields); @@ -438,8 +445,9 @@ public List> getContentsDao(String search, String order, int throw new IllegalArgumentException("order must be [table.]field [asc|desc]"); } } + // TODO: SAK-32704 - Resolve the different ways to do search search = foorm.searchCheck(search, "lti_content", fields); - + List> contents = getThingsDao("lti_content", LTIService.CONTENT_MODEL, extraSelect, joinClause, search, null, order, first, last, siteId, isAdminRole); for (Map content : contents) { content.put("launch_url", getContentLaunch(content)); @@ -448,17 +456,20 @@ public List> getContentsDao(String search, String order, int } /** - * + * * {@inheritDoc} * * @see org.sakaiproject.lti.api.LTIService#countContentsDao(java.lang.String, * java.lang.String, boolean) */ public int countContentsDao(String search, String siteId, boolean isAdminRole) { - String joinClause = "JOIN SAKAI_SITE ON lti_content.SITE_ID = SAKAI_SITE.SITE_ID" + // It is important that any tables/columns added for the purposes of display or searching be + // LEFT JOIN - *any* INNER JOIN will function as a WHERE clause and will hide content + // items from the admin UI - so they will not be seen and cannot be repaired + String joinClause = "LEFT JOIN SAKAI_SITE ON lti_content.SITE_ID = SAKAI_SITE.SITE_ID" + " LEFT JOIN SAKAI_SITE_PROPERTY ssp1 ON (lti_content.SITE_ID = ssp1.SITE_ID AND ssp1.name = 'contact-name')" + " LEFT JOIN SAKAI_SITE_PROPERTY ssp2 ON (lti_content.SITE_ID = ssp2.SITE_ID AND ssp2.name = 'contact-email')" - + " JOIN lti_tools ON (lti_content.tool_id = lti_tools.id)"; + + " LEFT JOIN lti_tools ON (lti_content.tool_id = lti_tools.id)"; final String propertyKey = serverConfigurationService.getString(LTI_SITE_ATTRIBUTION_PROPERTY_KEY, LTI_SITE_ATTRIBUTION_PROPERTY_KEY_DEFAULT); if (StringUtils.isNotEmpty(propertyKey)) { joinClause = joinClause + " LEFT JOIN SAKAI_SITE_PROPERTY ssp3 ON (lti_content.SITE_ID = ssp3.SITE_ID AND ssp3.name = '" + propertyKey + "')"; diff --git a/basiclti/basiclti-tool/src/bundle/ltitool.properties b/basiclti/basiclti-tool/src/bundle/ltitool.properties index 9f038de82f84..6b666c1de259 100644 --- a/basiclti/basiclti-tool/src/bundle/ltitool.properties +++ b/basiclti/basiclti-tool/src/bundle/ltitool.properties @@ -161,6 +161,8 @@ deploy.activate.notools=There are no tools to activate deploy.parse.exception=Unable to parse provider profile deploy.delete=Deleting Deployment deploy.delete.sure=Are you sure you want to delete this deployment? +deploy.delete.tool=Delete: +deploy.delete.tools=The tools below were installed by this deployment. If you delete these tools, all of the links to these tools in all the sites in this system will also be deleted. You can delete the deployment without deleting the tools. deploy.data=This deployment supports :tools tools and :contents placements in :sites sites. add.deploy=Install LTI 2.x Tool deploy.description.system=Deployments currently in this system diff --git a/basiclti/basiclti-tool/src/java/org/sakaiproject/blti/tool/LTIAdminTool.java b/basiclti/basiclti-tool/src/java/org/sakaiproject/blti/tool/LTIAdminTool.java index fff403abe0e0..13f1ab687c5e 100644 --- a/basiclti/basiclti-tool/src/java/org/sakaiproject/blti/tool/LTIAdminTool.java +++ b/basiclti/basiclti-tool/src/java/org/sakaiproject/blti/tool/LTIAdminTool.java @@ -746,42 +746,19 @@ public void doToolDelete(RunData data, Context context) return; } Long key = new Long(id); - if ( ltiService.deleteTool(key, getSiteId(state)) ) - { - state.setAttribute(STATE_SUCCESS,rb.getString("success.deleted")); - - // remove all content object and site links if any - // this is for the "site tools" panel - List> contents = ltiService.getContents(null,null,0,5000, getSiteId(state)); - for ( Map content : contents ) { - - Long tool_id_long = null; - try{ - tool_id_long = new Long(content.get(LTIService.LTI_TOOL_ID).toString()); - if (tool_id_long.equals(key)) - { - // the content with same tool id - // remove the content link first - String content_id = content.get(LTIService.LTI_ID).toString(); - Long content_key = content_id == null ? null:new Long(content_id); - - //TODO: how to handle the errors in content link and content deletion? - // remove the external tool content site link - ltiService.deleteContentLink(content_key, getSiteId(state)); - // remove the external tool content - ltiService.deleteContent(content_key, getSiteId(state)); - } - } - catch (Exception e) - { - // log the error - M_log.error("error parsing tool id " + content.get(LTIService.LTI_TOOL_ID)); - } - } - + + // Delete the tool and all associated content items and site links + List errors = ltiService.deleteToolAndContents(key, getSiteId(state)); + String errorNote = ""; + for(String errstr: errors ) { + M_log.error(errstr); + errorNote += "
"+errstr; + } + + if ( errors.size() < 1 ) { switchPanel(state, "ToolSystem"); } else { - addAlert(state,rb.getString("error.delete.fail")); + addAlert(state,rb.getString("error.delete.fail")+errorNote); switchPanel(state, "ToolSystem"); } } @@ -1495,6 +1472,10 @@ public String buildDeployDeletePanelContext(VelocityPortlet portlet, Context con return "lti_deploy_system"; } + // Retrieve all the tools that are related to this deploy + List> tools = ltiService.getTools("deployment_id = "+key,null,0,0, getSiteId(state)); + context.put("tools", tools); + Map deploy = deploys.get(0); context.put("deploy",deploy); @@ -1531,6 +1512,29 @@ public void doDeployDelete(RunData data, Context context) return; } Long key = new Long(id); + + // Retrieve all the tools that are related to this deploy + List> tools = ltiService.getTools("deployment_id = "+key,null,0,0, getSiteId(state)); + String errmsg = ""; + for ( Map tool : tools ) { + Long tool_id = foorm.getLongNull(tool.get("id")); + String parm = "delete_"+tool_id; + String val = data.getParameters().getString(parm); + if ( val == null || ! "on".equals(val) ) continue; + // Delete the tool and all associated content items and site links + List errors = ltiService.deleteToolAndContents(tool_id, getSiteId(state)); + for(String errstr: errors ) { + M_log.error(errstr); + errmsg += "
"+errstr; + } + } + + if ( errmsg.length() > 0 ) { + addAlert(state,rb.getString("error.delete.fail")+errmsg); + switchPanel(state, "DeploySystem"); + return; + } + // also remove the link if ( ltiService.deleteDeployDao(key) ) { diff --git a/basiclti/basiclti-tool/src/webapp/vm/lti_deploy_delete.vm b/basiclti/basiclti-tool/src/webapp/vm/lti_deploy_delete.vm index d85ed9952fc1..c251b4f0daa2 100644 --- a/basiclti/basiclti-tool/src/webapp/vm/lti_deploy_delete.vm +++ b/basiclti/basiclti-tool/src/webapp/vm/lti_deploy_delete.vm @@ -7,7 +7,24 @@
$tlang.getString("deploy.delete.sure")
$formOutput +

$deployData +

+#if ($tools.size() > 0) +

+

+ $tlang.getString("deploy.delete.tools") +
+

+
    + #foreach($tool in $tools) +
  • + + +
    • + #end +
+#end