Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename method that deletes item #35526

Merged
merged 2 commits into from
Jan 2, 2025
Merged

Rename method that deletes item #35526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74c282a
Rename method that deletes item
gherceg Dec 16, 2024
8eb4527
Raise exception instead of custom dictionary
gherceg Jan 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion corehq/apps/accounting/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ def get_create_item_data(self, create_form):
'template': 'accounting-admin-new',
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yes.

Copy link
Contributor

@millerdev millerdev Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

user = User.objects.get(id=item_id)
ops_role = Role.objects.get(slug=privileges.OPERATIONS_TEAM)
grant_to_remove = Grant.objects.filter(
Expand Down
8 changes: 4 additions & 4 deletions corehq/apps/data_interfaces/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ def get_create_item_data(self, create_form):
'template': 'new-group-template',
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
case_group = CommCareCaseGroup.get(item_id)
item_data = self._get_item_data(case_group)
case_group.soft_delete()
Expand Down Expand Up @@ -474,7 +474,7 @@ def get_create_item_data(self, create_form):
'template': 'new-case-template',
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
if not item_id:
raise PaginatedItemException("The case's ID was blank.")
current_cases = set(self.case_group.cases)
Expand Down Expand Up @@ -1067,10 +1067,10 @@ def activate_response(self):
def deactivate_response(self):
return self.update_rule()

def get_deleted_item_data(self, rule_id):
def delete_item(self, rule_id):
(rule, error) = self._get_rule(rule_id)
if rule is None:
return {'success': False, 'error': error}
raise PaginatedItemException(error)

rule.soft_delete()

Expand Down
6 changes: 3 additions & 3 deletions corehq/apps/hqwebapp/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,7 @@ def refresh_response(self):
@property
def delete_response(self):
try:
response = self.get_deleted_item_data(self.item_id)
response = self.delete_item(self.item_id)
return {
'deletedItem': response
}
Expand Down Expand Up @@ -1253,7 +1253,7 @@ def get_updated_item_data(self, update_form):
"""
raise NotImplementedError("You must implement get_updated_item_data")

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
"""
This should return a dict of data for the deleted item.
{
Expand All @@ -1264,7 +1264,7 @@ def get_deleted_item_data(self, item_id):
'template': <knockout template id>
}
"""
raise NotImplementedError("You must implement get_deleted_item_data")
raise NotImplementedError("You must implement delete_item")


@login_required
Expand Down
2 changes: 1 addition & 1 deletion corehq/apps/reminders/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ def _fmt_deleted_keyword_data(self, keyword):
'description': keyword.description,
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
try:
k = Keyword.objects.get(couch_id=item_id)
except Keyword.DoesNotExist:
Expand Down
2 changes: 1 addition & 1 deletion corehq/apps/reports/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1933,7 +1933,7 @@ def _get_item_data(self, tableau_visualization):
}
return data

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
tableau_viz = TableauVisualization.objects.get(
pk=item_id,
domain=self.domain,
Expand Down
2 changes: 1 addition & 1 deletion corehq/apps/settings/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ def get_create_item_data(self, create_form):
'template': 'new-user-api-key-template',
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
deleted_key = self.base_query.get(id=item_id)
deleted_key.delete()
return {
Expand Down
4 changes: 2 additions & 2 deletions corehq/apps/sms/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,7 @@ def _get_backend_from_item_id(self, item_id):
except (BadSMSConfigException, SQLMobileBackend.DoesNotExist, TypeError, ValueError):
raise Http404()

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
item_id, backend = self._get_backend_from_item_id(item_id)

if backend.is_global or backend.domain != self.domain:
Expand Down Expand Up @@ -1455,7 +1455,7 @@ def _get_backend_from_item_id(self, item_id):
except (BadSMSConfigException, SQLMobileBackend.DoesNotExist, TypeError, ValueError):
raise Http404()

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
item_id, backend = self._get_backend_from_item_id(item_id)

if not backend.is_global:
Expand Down
2 changes: 1 addition & 1 deletion corehq/apps/userreports/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1914,7 +1914,7 @@ def get_create_item_data(self, create_form):
"template": "base-ucr-statement-template",
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
deleted_expression = self.base_query.get(id=item_id)
deleted_expression.delete()
return {
Expand Down
4 changes: 2 additions & 2 deletions corehq/motech/dhis2/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def _get_item_data(self, dataset_map):
),
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
dataset_map = SQLDataSetMap.objects.get(domain=self.domain, pk=item_id)
dataset_map.delete()
return {
Expand Down Expand Up @@ -312,7 +312,7 @@ def get_updated_item_data(self, update_form):
"template": "datavalue-map-template",
}

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
datavalue_map = SQLDataValueMap.objects.get(pk=item_id,
dataset_map=self.object)
datavalue_map.delete()
Expand Down
2 changes: 1 addition & 1 deletion corehq/motech/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ def _get_item_data(self, connection_settings):

return data

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
connection_settings = ConnectionSettings.objects.get(
pk=item_id,
domain=self.domain,
Expand Down
2 changes: 1 addition & 1 deletion docs/ui_helpers.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ If you want to delete data with your paginated view, you should implement someth
class PuppiesCRUDView(BaseSectionView, CRUDPaginatedMixin):
...

def get_deleted_item_data(self, item_id):
def delete_item(self, item_id):
deleted_puppy = Puppy.get(item_id)
deleted_puppy.delete()
return {
Expand Down
Loading