10781 Directs graph.delete_instances to call bulk_delete methods #10782
Conversation
whatisgalen
changed the title
|
|
Good call! |
| @@ -563,19 +563,18 @@ def delete(self): | |||
| ) | |||
| ) | |||
|
|
|||
| def delete_instances(self, verbose=False): | |||
| def delete_instances(self, userid=None, verbose=False): | |||
There was a problem hiding this comment.
I make a habit of adding new optional arguments to the end of signatures to improve backwards compatibility.
There was a problem hiding this comment.
If they're both kwargs does order still matter?
There was a problem hiding this comment.
Yeah, unless they're keyword-only with *, to consume the positional args, which we don't have here. I floated starting to do this and got feedback that we don't do it anywhere else in Arches so not to bother with it.
There was a problem hiding this comment.
@whatisgalen I don't plan on blocking a merge over it, but did you have further thoughts?
There was a problem hiding this comment.
in my humblest of opinions on kwarg order, userid makes more sense as an earlier argument since it's more important. I'm not married to it but would be great to merge this sooner rather than later
There was a problem hiding this comment.
Agree that having verbose=False first is weird. I think arches-wide we should be open to the use of keyword-only arguments with * so that we don't end up introducing breaking changes like this. I'll consider writing a forum post.
In the meantime, I'll push a small breaking change notice to 7.6.0.md.
There was a problem hiding this comment.
Finally got around to the forum post: https://community.archesproject.org/t/preferring-keyword-only-parameters-when-adding-functionality-to-existing-functions/2538
whatisgalen
changed the title
There was a problem hiding this comment.
Thanks for the test cleanup, they look great. ⭐
Heads up, I pushed two cleanup commits to your branch including the release note I mentioned, so you'll wanna pull.
Noticed a failure when performing a manual test:
- Graph Designer > Delete Associated Instances
2024-05-02 10:49:26 File "/web_root/arches/arches/app/views/graph.py", line 472, in delete
2024-05-02 10:49:26 resp = graph.delete_instances(userid=request.user.id)
2024-05-02 10:49:26 File "/web_root/arches/arches/app/models/graph.py", line 576, in delete_instances
2024-05-02 10:49:26 bulk_deleter.index_resource_deletion(loadid)
2024-05-02 10:49:26 TypeError: BulkDataDeletion.index_resource_deletion() missing 1 required positional argument: 'resourceids'…with None default; refactors usage, re #10781
…archesproject/arches into 10781_show_deleted_instances_ct merges latest changes on remote branch
I neglected to refactor the signature for the bulk index method so that |
| bulk_deleter = BulkDataDeletion() | ||
| loadid = uuid.uuid4() | ||
| resp = bulk_deleter.delete_resources(userid, loadid, graphid=self.graphid, verbose=verbose) | ||
| bulk_deleter.index_resource_deletion(loadid) |
There was a problem hiding this comment.
This will drive more traffic into #10872, but that's not something that needs to block this work. Just FYI.
|
Taking a final look |
| result["success"] = True | ||
| result["deleted_count"] = deleted_count | ||
| result["message"] = _("Successfully deleted {} resources").format(str(deleted_count)) |
There was a problem hiding this comment.
I figure this is tolerable if there are 0 resources.
| if resourceids and graphid: | ||
| resources = Resource.objects.filter(graph_id=graphid).filter(pk__in=resourceids) | ||
| if resourceids: | ||
| resources = Resource.objects.filter(pk__in=resourceids) |
There was a problem hiding this comment.
I think this slightly degrades performance if both resources and graph are provided, so I don't see the readability tradeoff as worth it, but I don't see a current code path that exercises it, so I'm not too bothered.
Types of changes
Description of Change
This PR removes bespoke deletion logic for
Graph.delete_instances()and instead has it call the appropriate methods fromBulkDataDeleter. It also returns a count of deleted resources to the frontend.Issues Solved
#10781
Checklist
Ticket Background
Further comments