SSHOC / sshoc-marketplace-backend

Code for the backend
Apache License 2.0
2 stars 0 forks source link

Introduce a force parameter to delete actors #388

Closed KlausIllmayer closed 10 months ago

KlausIllmayer commented 1 year ago

We can delete actors that have never been applied to an item and when merging actors, the ones that are merged will be also deleted. What we miss is the option to delete actors that are currently not applied to any item but once have been applied to an item. We currently don't have an option to delete such actors.

We once discussed this (see #143) but the issue was closed (I guess we thought that it is not necessary). In the meantime we came to the conclusion, that for clean-up reasons it could be indeed useful, to have a parameter "force" (as we have it in some other API calls, especially for vocabulary concepts) that allows to delete such actors. We manipulate the history of items with such an option, but it should be really limited only to actors, that do not have any active relation to an item.

The additional force parameter should cover this:

What do you think @tparkola @laureD19 ?

laureD19 commented 1 year ago

Taking into account "active"/"inactive" versions of items. Deletion of actors should only be possible for "inactive"versions of items.

tparkola commented 11 months ago

The parameter is introduced. It can be used only by administrators. In addition, the actor (that is to be deleted) cannot be linked to an active version of an item. The result is that the actor disappears from the history of all versions of items.

KlausIllmayer commented 11 months ago

@tparkola One question: you write that "adminstrators plus the actor" can delete an actor - I'm not sure, how this could work, as we separate users from actors in the database. I think a user can only act as informationContributor but not as an actor. Looking into the code it seems to me that only administrators are allowed to do this, right?

KlausIllmayer commented 11 months ago

An open issue here is that when deleting an actor who is still a contributor of an active item you get a not very nice error message when using force=false :

{
  "timestamp": "2023-08-18 14:52:21",
  "status": 500,
  "error": "could not execute statement; SQL [n/a]; constraint [item_contributor_actor_id_fk]; nested exception is org.hibernate.exception.ConstraintViolationException: could not execute statement"
}

You can try it out on stage with DELETE /api/actors/1763?force=false. If you use force=true the error message is way more nicer:

{
  "timestamp": "2023-08-18 15:01:29",
  "status": 400,
  "error": "Cannot delete actors that are contributors of active items!"
}

@tparkola Can we also give this error message when using force=false (which is also the default when not explicit adding force)? And do you prefer to have this in a dedicated issue or can you re-use this issue?

KlausIllmayer commented 11 months ago

There is also the special case where an actor is not applied to any item but it is the affiliation of other actors. It would be great to have a nice error message for such cases, currently it returns:

{
  "timestamp": "2023-08-18 15:45:05",
  "status": 500,
  "error": "could not execute statement; SQL [n/a]; constraint [actors_affiliations_affiliation_id_fk]; nested exception is org.hibernate.exception.ConstraintViolationException: could not execute statement"
}

On stage you can try it out with DELETE /api/actors/990?force=true. @tparkola Can we have instead a 400 status with error "Cannot delete actors that are affiliations of other actors"?

KlausIllmayer commented 11 months ago

Tested it by creating a tool, applying a new actor, publish it => can't delete actor as it is connected to an active item. Now created a new version of this tool and deleted the actor from it => now I can delete the actor with force=true, the actor disappears from the history. Doing this by creating a tool, applying a new actor, deleting the tool also worked.

tparkola commented 11 months ago

@tparkola One question: you write that "adminstrators plus the actor" can delete an actor - I'm not sure, how this could work, as we separate users from actors in the database. I think a user can only act as informationContributor but not as an actor. Looking into the code it seems to me that only administrators are allowed to do this, right?

I edited my comment, so that it better explains what I meant. I hope now it is clear.

tparkola commented 11 months ago

There is also the special case where an actor is not applied to any item but it is the affiliation of other actors. It would be great to have a nice error message for such cases, currently it returns:

{
  "timestamp": "2023-08-18 15:45:05",
  "status": 500,
  "error": "could not execute statement; SQL [n/a]; constraint [actors_affiliations_affiliation_id_fk]; nested exception is org.hibernate.exception.ConstraintViolationException: could not execute statement"
}

On stage you can try it out with DELETE /api/actors/990?force=true. @tparkola Can we have instead a 400 status with error "Cannot delete actors that are affiliations of other actors"?

I will fix it here, no problem.

tparkola commented 11 months ago

There is also the special case where an actor is not applied to any item but it is the affiliation of other actors. It would be great to have a nice error message for such cases, currently it returns:

{
  "timestamp": "2023-08-18 15:45:05",
  "status": 500,
  "error": "could not execute statement; SQL [n/a]; constraint [actors_affiliations_affiliation_id_fk]; nested exception is org.hibernate.exception.ConstraintViolationException: could not execute statement"
}

On stage you can try it out with DELETE /api/actors/990?force=true. @tparkola Can we have instead a 400 status with error "Cannot delete actors that are affiliations of other actors"?

Yes, will do so.

KlausIllmayer commented 10 months ago

Unfortunately, by introducing the conditional "Cannot delete actors that are contributors of items!" - in code review - it is not possible anymore to delete once applied actors as described in https://github.com/SSHOC/sshoc-marketplace-backend/issues/388#issuecomment-1684126204. the reason seems to me that in the corresponding query you don't limit to active items thus it will not allow you to delete an actor (I think this looks like a working example: code)

if you create an item and apply a new actor, then create a new version and delete the actor to the item, you won't be able to delete this actor afterwards. but it should be possible, as the actor is not applied to an active version of the item. the same is true if you delete the item. it won't allow you the actor neither.

tparkola commented 10 months ago

Did you use force=true? I've just tested the scenario you described and it works in my local environment (using develop branch and 388 branch). Could you please double check and let me know? The code you refer to ("in code review") is executed if the force flag is false.

KlausIllmayer commented 10 months ago

yes, you are right - sorry about that, everything works as expected