Added code for deleting individuals #213

litvinovg · 2021-02-19T10:35:14Z

What does this pull request do?

Adds functionality to delete individuals and specify SPARQL Query to also delete individual dependant objects.
Require documentation update to provide examples with such queries.

What's new?

DeleteIndividualController to execute deletion
Display data property display:hasDeleteQuery to specify CONSTRUCT SPARQL query for a class to get all dependant object at deletion phase.
If class has no associated delete query, then default will be used.

How should this be tested?

Add <@p.deleteIndividualLink individual /> to individual page to create a link for individual deletion. Use link to delete individual.
Write CONSTRUCT query in format display:hasDeleteQuery """ construct query text """ .
and save it to rdf/display/everytime/

Interested parties

Tag (@ mention) interested parties or, if unsure, @VIVO-project/vivo-committers

chenejac

@litvinovg thanks for nice contribution. I have posted a couple of comments. Moreover, I have two additional comments here (and one in the associated GitHub issue):

Please, consider adding <@p.deleteIndividualLink individual /> in the webapp/src/main/webapp/templates/freemarker/body/partials/individual/individual-adminPanel.ftl next to link for editing individual
I am not sure do we need anymore Delete button in webapp/src/main/webapp/templates/edit/formBasic.jsp. If we do, it should be linked to the same code, i.e., display:hasDeleteQuery should be taken into account.

chenejac · 2021-12-21T09:59:03Z

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

+  }
+
+  private void deleteIndividuals(byte[] toRemove, VitroRequest vreq) {
+    String removingString = new String(toRemove, StandardCharsets.UTF_8);


Can this line be moved to the catch block?

chenejac · 2021-12-21T10:07:38Z

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

+    return type;
+  }
+
+  private byte[] getIndividualsToDelete(String targetIndividual, String deleteQuery, VitroRequest vreq) {


I am not sure the best return value of this function is byte[]. Can you consider encapsulating conversion of Model to byte[] and creation of InputStream within the deleteIndividuals method?

chenejac · 2021-12-21T10:09:33Z

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

+      QueryExecution qexec = QueryExecutionFactory.create(queryForTypeSpecificDeleteQuery, displayModel, initialBindings);
+      try {
+        ResultSet results = qexec.execSelect();
+        while (results.hasNext()) {


Can here be if instead of while?

Yes. It is more intuitive.

chenejac · 2021-12-21T10:17:04Z

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

+    String redirectUrl = vreq.getParameter("redirectUrl");
+    if (redirectUrl != null) {
+      return redirectUrl;
+    }
+    return "/";


Can confirmDeleteIndividualForm.ftl (lines 5-9) be simplified? Do we need if clause there, or
<#assign redirectUrl = editConfiguration.pageData.redirectUrl />
is enough? Or we can simplify the code here in the Java file?

I think validation is needed as it get values from the browser in both cases. Assigning limits configuration of redirect url. I would leave it that way.

chenejac · 2021-12-21T10:18:23Z

...ell/mannlib/vitro/webapp/edit/n3editing/configuration/generators/DefaultDeleteGenerator.java

+  private String propertyTemplate = "confirmDeletePropertyForm.ftl";
+  private String individualTemplate = "confirmDeleteIndividualForm.ftl";


Can you please correct lay outing of those two lines?

brianjlowe

Great addition. Just a few little things to tweak and then a slightly bigger question about how we want to handle the permissions.

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

brianjlowe · 2022-02-10T07:32:13Z

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

+
+	@Override
+	protected AuthorizationRequest requiredActions(VitroRequest vreq) {
+		return SimplePermission.DO_FRONT_END_EDITING.ACTION;


This is something that a lot of existing controllers tend not to handle well, but which we should probably consider more closely. It seems that this controller should require the user to have permission at least for edu.cornell.mannlib.vitro.webapp.auth.requestedAction.resource.DropResource for the main individualURI that is deleted. The thornier question is what to do with the related resources and (possibly the specific statements connecting them). Do we want to pre-run the deletion query and check each of the additional individual URIs it returns to see whether the user also has DropResource permissions on those? If a user has permission to drop the main resource but not some of the related individuals, do we fail on the whole operation or allow them to delete those individuals they are able to while leaving the rest behind?

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

brianjlowe · 2022-02-10T07:40:52Z

...du/cornell/mannlib/vitro/webapp/edit/n3editing/controller/EditRequestDispatchController.java

@@ -75,6 +75,9 @@ protected AuthorizationRequest requiredActions(VitroRequest vreq) {
 		if (StringUtils.isNotEmpty(EditConfigurationUtils.getTypeOfNew(vreq))) {
 			return SimplePermission.DO_BACK_END_EDITING.ACTION;
 		}
+		if (isIndividualDeletion(vreq)) {
+			return SimplePermission.DO_BACK_END_EDITING.ACTION;


See comment on requiredActions() in DeleteIndividualController.

.../java/edu/cornell/mannlib/vitro/webapp/controller/freemarker/DeleteIndividualController.java

…tic, lookup for mostSpecificTypes

…itable

litvinovg · 2022-04-11T16:46:26Z

@litvinovg thanks for nice contribution. I have posted a couple of comments. Moreover, I have two additional comments here (and one in the associated GitHub issue):
1. Please, consider adding <@p.deleteIndividualLink individual /> in the webapp/src/main/webapp/templates/freemarker/body/partials/individual/individual-adminPanel.ftl next to link for editing individual

I added the link.

2. I am not sure do we need anymore Delete button in webapp/src/main/webapp/templates/edit/formBasic.jsp. If we do, it should be linked to the same code, i.e., display:hasDeleteQuery should be taken into account.

I don't think we are ready to do it now, but we should consider that later.

chenejac · 2022-04-11T20:44:05Z

@litvinovg thanks for nice contribution. I have posted a couple of comments. Moreover, I have two additional comments here (and one in the associated GitHub issue):
1. Please, consider adding <@p.deleteIndividualLink individual /> in the webapp/src/main/webapp/templates/freemarker/body/partials/individual/individual-adminPanel.ftl next to link for editing individual
I added the link.
2. I am not sure do we need anymore Delete button in webapp/src/main/webapp/templates/edit/formBasic.jsp. If we do, it should be linked to the same code, i.e., display:hasDeleteQuery should be taken into account.
I don't think we are ready to do it now, but we should consider that later.

chenejac · 2022-04-11T20:46:21Z

I am not sure do we need anymore Delete button in webapp/src/main/webapp/templates/edit/formBasic.jsp. If we do, it should be linked to the same code, i.e., display:hasDeleteQuery should be taken into account.
I don't think we are ready to do it now, but we should consider that later.

@litvinovg thanks for addressing the first comment. For the second comment, I suggest then to open an issue about this, and to work on that in the future.

brianjlowe

Looks good. Confirmed that it logs an error and uses the default deletion behavior if the ?individualUri query is not present. Query works when correct variable name is used.

* Added code for deleting individuals * Allow admins delete individuals * Fixed delete individual url params * Improved query safety * refact: fixed constants' names, fixed mistakes, removed useless code * refact: refactored sparql queries * fix: fixed error logging mistakes * fix: use CONSTRUCT instead of DESCRIBE as less potentionally problematic, lookup for mostSpecificTypes * feat: add delete link in vitro individual profile if individual is editable * fix: renamed individualURI to individualUri and added safety check * fix: use ABox model to construct triples to delete

litvinovg mentioned this pull request Feb 19, 2021

Added english translation for individual deletion confirm form vivo-project/Vitro-languages#44

Merged

chenejac mentioned this pull request Oct 12, 2021

VIVO-1962: Improvements to removing individuals vivo-project/VIVO#3548

Closed

chenejac requested review from chenejac and brianjlowe December 14, 2021 09:59

chenejac added the middle PR A middle pull request label Dec 14, 2021

chenejac requested a review from gneissone December 14, 2021 10:11

chenejac requested changes Dec 21, 2021

View reviewed changes

chenejac previously approved these changes Jan 7, 2022

View reviewed changes

chenejac removed the request for review from gneissone January 27, 2022 15:54

brianjlowe requested changes Feb 10, 2022

View reviewed changes

litvinovg added 8 commits April 8, 2022 14:41

Added code for deleting individuals

e25c776

Allow admins delete individuals

8b76b27

Fixed delete individual url params

3f43f47

Improved query safety

1b7c157

refact: fixed constants' names, fixed mistakes, removed useless code

9d58b6a

refact: refactored sparql queries

4e0cb7c

fix: fixed error logging mistakes

6c83fcf

fix: use CONSTRUCT instead of DESCRIBE as less potentionally problema…

bc4e93a

…tic, lookup for mostSpecificTypes

litvinovg dismissed chenejac’s stale review via bc4e93a April 8, 2022 15:07

litvinovg force-pushed the del_individuals_main branch from 1ed9a27 to bc4e93a Compare April 8, 2022 15:07

feat: add delete link in vitro individual profile if individual is ed…

cef5bab

…itable

chenejac closed this Apr 11, 2022

chenejac reopened this Apr 11, 2022

chenejac previously approved these changes Apr 12, 2022

View reviewed changes

fix: renamed individualURI to individualUri and added safety check

fd25616

litvinovg dismissed chenejac’s stale review via fd25616 May 10, 2022 13:35

fix: use ABox model to construct triples to delete

9a8a58a

brianjlowe approved these changes May 20, 2022

View reviewed changes

chenejac merged commit ff285fb into vivo-project:main May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added code for deleting individuals #213

Added code for deleting individuals #213

litvinovg commented Feb 19, 2021 •

edited

chenejac left a comment

chenejac Dec 21, 2021

litvinovg Jan 6, 2022

chenejac Dec 21, 2021

litvinovg Jan 6, 2022

chenejac Dec 21, 2021

litvinovg Jan 6, 2022

chenejac Dec 21, 2021

litvinovg Jan 7, 2022

chenejac Dec 21, 2021

litvinovg Jan 6, 2022

brianjlowe left a comment

brianjlowe Feb 10, 2022

brianjlowe Feb 10, 2022

litvinovg commented Apr 11, 2022 •

edited

chenejac commented Apr 11, 2022

chenejac commented Apr 11, 2022

brianjlowe left a comment

		private String propertyTemplate = "confirmDeletePropertyForm.ftl";
		private String individualTemplate = "confirmDeleteIndividualForm.ftl";

Added code for deleting individuals #213

Added code for deleting individuals #213

Conversation

litvinovg commented Feb 19, 2021 • edited

What does this pull request do?

What's new?

How should this be tested?

Interested parties

chenejac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjlowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litvinovg commented Apr 11, 2022 • edited

chenejac commented Apr 11, 2022

chenejac commented Apr 11, 2022

brianjlowe left a comment

Choose a reason for hiding this comment

litvinovg commented Feb 19, 2021 •

edited

litvinovg commented Apr 11, 2022 •

edited