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.

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

Delete Case Types and Properties in Data Dictionary #33956

Closed
wants to merge 57 commits into from

Conversation

zandre-eng
Copy link
Contributor

@zandre-eng zandre-eng commented Jan 8, 2024

Product Description

There is a new button to delete a case property. This button is contextual and will only appear for case properties that are not being used to store actual case data:
case-prop-delete-button

Clicking on the case property delete button will cause a confirmation modal to pop up:
delete-case-prop-modal

After the user has clicked to confirm deletion, the case property will then be marked as deleted. The user will then need to confirm all changes by clicking "Save" at the top-right corner. This follows the same workflow as deprecating a case property.

Similarly, there is a new button to delete a case type. This button is also contextual and only appears if both the case type and all its case properties are not being used by cases:
delete-case-type-button

Clicking the case type delete button will cause a confirmation modal to pop up:
delete-case-type-modal

Once the user has confirmed to delete a case type, the page will be refreshed and the deleted case type will be gone from the left-hand side.

If there is an error with deleting a case type, a generic error message will be shown to the user:
delete-case-type-error

Technical Summary

Link to ticket here.

This adds in the ability for a user to delete case types/properties. It should be noted however, that this only applies to case types/properties that have no associated case data. Once a case starts using a specific case type/property, the type/property can then no longer be deleted in the Data Dictionary. The only way to achieve deleting it would be to first remove the relevant cases.

Feature Flag

None.

Safety Assurance

Safety story

  • Local testing.
  • Will go through QA.
  • Automated tests.

Automated test coverage

Automated tests have been created for the new view and util functions for deleting a case type. Furthermore, the existing unit tests for updating a case property have been updated to test for deleting case properties as well.

QA Plan

QA has passed. Ticket available here.

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

@zandre-eng zandre-eng added the product/all-users-all-environments Change impacts all users on all environments label Jan 8, 2024
@require_permission(HqPermissions.edit_data_dict)
def delete_case_type(request, domain, case_type_name):
case_type_obj = CaseType.objects.get(domain=domain, name=case_type_name)
case_type_obj.delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we want to check for missing case type here like we did in delete_case_property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a good check to have. Although I don't foresee this scenario happening, there might be an unforeseen edge case and so we should rather stay on the cautious side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try:
prop = CaseProperty.objects.get(name=name, case_type__name=case_type, case_type__domain=domain)
except CaseProperty.DoesNotExist:
return gettext('Case property does not exist.')
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 see we have done it elsewhere. Should translations be done in the util module or should they be limited to views?
I guess I would go for latter but don't have a strong reason against it keeping it in util, specially if its already being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be fine to keep in the util module. There are other functions in this module, such as save_case_property(), which also use gettext.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right 👍

Copy link
Contributor

@mkangia mkangia left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback @zandre-eng
All non blocking feedback.

Just confirming that comms for this change have already been setup/addressed?

@zandre-eng zandre-eng added the Open for review: do not merge A work in progress label Feb 5, 2024
@zandre-eng
Copy link
Contributor Author

Given the size of this PR, I am closing it off in favour of this one which contains a cleaned up commit history.

@zandre-eng zandre-eng closed this Feb 7, 2024
@zandre-eng zandre-eng deleted the ze/delete-in-data-dictionary branch February 7, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Open for review: do not merge A work in progress product/all-users-all-environments Change impacts all users on all environments QA Passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants