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

Remove general purpose dedupe toggle #33963

Merged
merged 10 commits into from
Feb 15, 2024
Merged

Remove general purpose dedupe toggle #33963

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f50b500
gate dedupe updates behind existing toggle
mjriley Jan 11, 2024
14db22e
Remove general dedupe toggle
mjriley Jan 11, 2024
fe4d281
update dedupe list description
mjriley Jan 12, 2024
addc6ec
Merge branch 'mjr/create_dedupe_privilege' into mjr/remove-dedupe-toggle
mjriley Jan 15, 2024
d386ead
Merge branch 'mjr/create_dedupe_privilege' into mjr/remove-dedupe-toggle
mjriley Jan 17, 2024
e986e6c
require pro plan or above to access dedupe features
mjriley Jan 18, 2024
faef877
add a frozen toggle to support the previous CASE_DEDUPE toggle
mjriley Jan 19, 2024
35206f3
Merge branch 'mjr/dedupe_backfill' into mjr/remove-dedupe-toggle
mjriley Jan 19, 2024
1d531e2
Correctly gate dedupe actions behind the proper toggle
mjriley Jan 24, 2024
d7d4cbc
Merge branch 'mjr/dedupe_backfill' into mjr/remove-dedupe-toggle
mjriley Feb 15, 2024
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
6 changes: 5 additions & 1 deletion corehq/apps/data_interfaces/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
paginate_query,
paginate_query_across_partitioned_databases, create_unique_index_name,
)
from corehq import toggles
from corehq.util.log import with_progress_bar
from corehq.util.quickcache import quickcache
from corehq.util.test_utils import unit_testing_only
Expand Down Expand Up @@ -1178,7 +1179,10 @@ def _handle_case_duplicate_new(self, case, rule):
existing_duplicate.delete()
duplicate_ids = self._create_duplicates(case, rule, current_hash)

num_updates = self._update_duplicates(duplicate_ids, case, rule)
if toggles.CASE_DEDUPE.enabled(rule.domain):
num_updates = self._update_duplicates(duplicate_ids, case, rule)
else:
num_updates = 0
return CaseRuleActionResult(num_updates=num_updates)

def _create_duplicates(self, case, rule, current_hash):
Expand Down
3 changes: 0 additions & 3 deletions corehq/apps/data_interfaces/pillow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
from corehq.form_processor.models import CommCareCase
from corehq.form_processor.models.forms import XFormInstance
from corehq.apps.hqcase.constants import UPDATE_REASON_RESAVE
from corehq.toggles import CASE_DEDUPE
from corehq.util.soft_assert import soft_assert


Expand All @@ -28,8 +27,6 @@ class CaseDeduplicationProcessor(PillowProcessor):

def process_change(self, change):
domain = change.metadata.domain
if not CASE_DEDUPE.enabled(domain):
return

if change.deleted:
# TODO: If a case gets deleted, we don't remove the duplicates for this case?
Expand Down
5 changes: 1 addition & 4 deletions corehq/apps/data_interfaces/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
from corehq.apps.case_importer.do_import import SubmitCaseBlockHandler, RowAndCase
from corehq.motech.repeaters.models import SQLRepeatRecord
from corehq.sql_db.util import get_db_aliases_for_partitioned_query
from corehq.toggles import CASE_DEDUPE, DISABLE_CASE_UPDATE_RULE_SCHEDULED_TASK
from corehq.toggles import DISABLE_CASE_UPDATE_RULE_SCHEDULED_TASK
from corehq.util.celery_utils import no_result_task
from corehq.util.decorators import serial_task
from corehq.util.log import send_HTML_email
Expand Down Expand Up @@ -66,9 +66,6 @@ def _progress_tracker(current, total):
@no_result_task(queue='case_rule_queue', acks_late=True,
soft_time_limit=15 * settings.CELERY_TASK_SOFT_TIME_LIMIT)
def reset_and_backfill_deduplicate_rule_task(domain, rule_id):
if not CASE_DEDUPE.enabled(domain):
return

try:
rule = AutomaticUpdateRule.objects.get(
id=rule_id,
Expand Down
2 changes: 2 additions & 0 deletions corehq/apps/data_interfaces/templates/data_interfaces/edit_deduplication_rule.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
{% crispy criteria_form %}
</div>
</div>
{% if update_actions_enabled %}
<fieldset {% if readonly %} disabled="disabled" {% endif %}>
<legend>{% trans "Actions" %}</legend>
<div data-bind="template: {name: 'update-case-property-action', foreach: propertiesToUpdate, as: 'propertyToUpdate'} "></div>
Expand All @@ -101,6 +102,7 @@
<input type="text" hidden name="properties_to_update" data-bind="value: serializedPropertiesToUpdate" />
</div>
</fieldset>
{% endif %}
{% if not readonly %}
<div class="form-actions">
<div class="col-xs-1">
Expand Down
3 changes: 2 additions & 1 deletion corehq/apps/data_interfaces/tests/test_case_deduplication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ def test_find_duplicates_blank_case_properties(self):
find_duplicate_case_ids(self.domain, cases[0], ["name", "dob"], match_type="ANY"))


@flag_enabled('CASE_DEDUPE')
class CaseDeduplicationActionTest(TestCase):
def setUp(self):
super().setUp()
Expand Down Expand Up @@ -692,7 +693,6 @@ def test_rule_deletion(self):
self.rule.soft_delete()
self.assertEqual(CaseDuplicateNew.objects.filter(action=self.action).count(), 0)

@flag_enabled('CASE_DEDUPE')
def test_case_deletion(self):
"""Test that deleting cases also deletes Duplicate Relationships
"""
Expand Down Expand Up @@ -882,6 +882,7 @@ def _configure_properties_to_update(self, rule, prop_map):
action.save()


@flag_enabled('CASE_DEDUPE')
@es_test(requires=[case_search_adapter, user_adapter])
class TestDeduplicationRuleRuns(TestCase):
def setUp(self):
Expand Down
16 changes: 10 additions & 6 deletions corehq/apps/data_interfaces/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,6 @@ def read_only_mode(self):
return True


@method_decorator(toggles.CASE_DEDUPE.required_decorator(), name='dispatch')
class DeduplicationRuleListView(DataInterfaceSection, CRUDPaginatedViewMixin):
template_name = 'data_interfaces/list_deduplication_rules.html'
urlname = 'deduplication_rules'
Expand Down Expand Up @@ -1148,7 +1147,6 @@ def _get_duplicates_count(self, rule):
return CaseDuplicateNew.objects.filter(action=action).count()


@method_decorator(toggles.CASE_DEDUPE.required_decorator(), name='dispatch')
class DeduplicationRuleCreateView(DataInterfaceSection):
template_name = "data_interfaces/edit_deduplication_rule.html"
urlname = 'add_deduplication_rule'
Expand All @@ -1161,6 +1159,7 @@ def page_context(self):
'all_case_properties': self.get_augmented_data_dict_props_by_case_type(self.domain),
'case_types': sorted(list(get_case_types_for_domain(self.domain))),
'criteria_form': self.case_filter_form,
'update_actions_enabled': toggles.CASE_DEDUPE.enabled(self.domain),
})
return context

Expand Down Expand Up @@ -1250,11 +1249,17 @@ def parse_params(self, request):
if prop
],
"include_closed": request.POST.get("include_closed") == "on",
"properties_to_update": [
}
if toggles.CASE_DEDUPE.enabled(self.domain):
properties_to_update = [
{"name": prop["name"], "value_type": prop["valueType"], "value": prop["value"]}
for prop in json.loads(request.POST.get("properties_to_update"))
],
}
]
else:
properties_to_update = []

action_params["properties_to_update"] = properties_to_update

return rule_params, action_params

def validate_rule_params(self, domain, rule_params, rule=None):
Expand Down Expand Up @@ -1307,7 +1312,6 @@ def _create_rule(self, domain, name, case_type):
)


@method_decorator(toggles.CASE_DEDUPE.required_decorator(), name='dispatch')
class DeduplicationRuleEditView(DeduplicationRuleCreateView):
urlname = 'edit_deduplication_rule'
page_title = gettext_lazy("Edit Deduplication Rule")
Expand Down
3 changes: 1 addition & 2 deletions corehq/apps/users/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ def tag_cases_as_deleted_and_remove_indices(domain, case_ids, deletion_id, delet
_remove_indices_from_deleted_cases_task.delay(domain, case_ids)
delete_phone_numbers_for_owners.delay(case_ids)
delete_schedule_instances_for_cases.delay(domain, case_ids)
if toggles.CASE_DEDUPE.enabled(domain):
delete_duplicates_for_cases.delay(case_ids)
delete_duplicates_for_cases.delay(case_ids)


@task(serializer='pickle', rate_limit=2, queue='background_queue', ignore_result=True, acks_late=True)
Expand Down
3 changes: 1 addition & 2 deletions corehq/reports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ def REPORTS(project):
if toggles.CASE_LIST_EXPLORER.enabled(project.name) or domain_can_access_case_list_explorer:
inspect_reports.append(CaseListExplorer)

if toggles.CASE_DEDUPE.enabled(project.name):
inspect_reports.append(DuplicateCasesExplorer)
inspect_reports.append(DuplicateCasesExplorer)

deployments_reports = (
deployments.ApplicationStatusReport,
Expand Down
23 changes: 9 additions & 14 deletions corehq/tabs/tabclasses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,11 +574,6 @@ def can_view_ecd_preview(self):
return (EXPLORE_CASE_DATA_PREVIEW.enabled_for_request(self._request)
and is_eligible_for_ecd_preview(self._request))

@property
@memoized
def can_deduplicate_cases(self):
return toggles.CASE_DEDUPE.enabled_for_request(self._request)

@property
def _can_view_geospatial(self):
return toggles.GEOSPATIAL.enabled(self.domain)
Expand Down Expand Up @@ -943,15 +938,15 @@ def _get_edit_section(self):
else:
edit_section = [(gettext_lazy('Edit Data'), [automatic_update_rule_list_view])]

if self.can_deduplicate_cases:
from corehq.apps.data_interfaces.views import (
DeduplicationRuleListView,
)
deduplication_list_view = {
'title': _(DeduplicationRuleListView.page_title),
'url': reverse(DeduplicationRuleListView.urlname, args=[self.domain]),
}
edit_section[0][1].append(deduplication_list_view)
from corehq.apps.data_interfaces.views import (
DeduplicationRuleListView,
)
deduplication_list_view = {
'title': _(DeduplicationRuleListView.page_title),
'url': reverse(DeduplicationRuleListView.urlname, args=[self.domain]),
}
edit_section[0][1].append(deduplication_list_view)

return edit_section

def _get_explore_data_views(self):
Expand Down
2 changes: 1 addition & 1 deletion corehq/toggles/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1464,7 +1464,7 @@ def _commtrackify(domain_name, toggle_is_enabled):

CASE_DEDUPE = StaticToggle(
Copy link
Member

Choose a reason for hiding this comment

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

do we want this to be a FrozenPrivilegeToggle so that new users can't be added to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want this to be a FrozenPrivilegeToggle so that new users can't be added to it?

Thanks for pointing this class out to me. The immediate problem is that I don't believe there is an associated privilege for dedupe, which I'm making sure isn't an oversight on my part. Beyond that, I'm not sure this is a great fit. It sounds like FrozenPrivilegeToggle is meant to duplicate privilege functionality for domains that don't have the privilege. In this case, this toggle is meant to be for domains that are grandfathered into still allowing case updates, which the GA feature will not allow

Copy link
Contributor

Choose a reason for hiding this comment

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

this toggle is meant to be for domains that are grandfathered into...

I think this is exactly what FrozenPrivilegeToggle was intended for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify further? From that CEP, my understanding is that GA'd features become privileges on software plans. Existing domains that use the GA'd functionality may not belong to those software plans, and so FrozenPrivilegeToggle is intended to bridge that gap. But, whether it is the privilege or the toggle, both are supporting the exact same workflow.

That last part is why I don't believe FrozenPrivilegeToggle is a good fit here. We are discussing different functionality: the GA'd feature will not allow updates, while the existing toggle still will (in fact, the toggle's sole purpose will be to support updates). I think its also reasonable to suspect that this update functionality will be open to domains being added and removed, because while we're GA-ing 75% of the feature, I imagine Solutions still wants to support clients that require 100% of the feature.

'case_dedupe',
'Case deduplication feature',
'Allow Case deduplication update actions',
TAG_SOLUTIONS_LIMITED,
[NAMESPACE_DOMAIN],
help_link='https://confluence.dimagi.com/display/saas/Surfacing+Case+Duplicates+in+CommCare',
Expand Down