Rename method that deletes item

[gherceg]

Product Description

Technical Summary

I saw that this method deletes the item based on the item_id passed in, and then returns it in a specific format, and felt like this should be named more clearly.

Feature Flag

Safety Assurance

Safety story

Will roll out on staging first and do a quick check. This is a straightforward rename so I don't anticipate any issues.

Automated test coverage

QA Plan

No

Rollback instructions

• This PR can be reverted after deploy with no further considerations

Labels & Review

• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[mkangia]

nit: I like good naming.

I would expect delete_item to delete the item and return a boolean or the deleted item.
Though the return here isn't either of those which can be confusing and its pretty custom/special return here.

I wonder if the original naming is okay.

1. Considering it's a soft delete
2. Though mainly, some inherited classes are not deleting at all. So was it deleted elsewhere?

May be the implementation of this function could have been different to start with.
I'd suggest leaving it the way it is as it does not seem to be making it really better.

[gherceg]

Though the return here isn't either of those which can be confusing and its pretty custom/special return here.

Agreed that it is custom, but I think it is still in line with the convention of a delete method returning something related to the deleted object or operation. As far as knowing what exactly is returned, I'd argue that documentation on the base class method should be sufficient for a developer to figure that out.

    This should return a dict of data for the deleted item.
   {
       'itemData': {
           'id': <id of item>,
           <json dict of item data for the knockout model to use>
       },
       'template': <knockout template id>
   }

Though mainly, some inherited classes are not deleting at all. So was it deleted elsewhere?

Are you referring to the method in CaseGroupCaseManagementView? It does appear to be deleting the id from the list of cases in the case group?

        current_cases = set(self.case_group.cases)
        self.case_group.cases = list(current_cases.difference([item_id]))
        self.case_group.save()

I do agree that this isn't your average delete method, and therefore the updated name might be too generic. I'd be open to appending something like def delete_item_for_crud_view to ground this in the context of the paginated crud view mixin. I think anything indicating that an item is being deleted is a far better name than get_deleted_item_data.

[gherceg]

Oh there are definitely cases where the dictionary described in the method docs is not what is returned, which obviously is not ideal. Thinking of things like return {'success': False, 'error': error}. The better approach seems to be to raise an error with the error message, and let the caller handle accordingly, which other implementations do. I can make that change for consistency.

[orangejenny]

think anything indicating that an item is being deleted is a far better name than get_deleted_item_data

Yes.

[millerdev]

anything indicating that an item is being deleted is a far better name than get_deleted_item_data

+1

get methods generally should not change state.