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

Cleanup contact list pages and reduce db hits #5587

Merged
merged 2 commits into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 6 additions & 3 deletions temba/contacts/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -495,13 +495,16 @@ def get_or_create(cls, org, user, key: str, name: str = None, value_type=None):
)

@classmethod
def get_fields(cls, org: Org, viewable_by=None):
def get_fields(cls, org: Org, featured=None, viewable_by=None):
"""
Gets the fields for the given org
"""

fields = org.fields.filter(is_active=True, is_proxy=False)

if featured is not None:
fields = fields.filter(show_in_table=featured)

if viewable_by and org.get_user_role(viewable_by) == OrgRole.AGENT:
fields = fields.exclude(agent_access=cls.ACCESS_NONE)

Expand Down Expand Up @@ -836,7 +839,7 @@ def get_field_serialized(self, field) -> str:

return value_dict.get(engine_type)

def get_field_value(self, field):
def get_field_value(self, field: ContactField):
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need type annotations everywhere.. but like here it's helpful to know this isn't the key string but is the actual field object

"""
Given the passed in contact field object, returns the value (as a string, decimal, datetime, AdminBoundary)
for this contact or None.
Expand All @@ -858,7 +861,7 @@ def get_field_value(self, field):
elif field.value_type in [ContactField.TYPE_STATE, ContactField.TYPE_DISTRICT, ContactField.TYPE_WARD]:
return AdminBoundary.get_by_path(self.org, string_value)

def get_field_display(self, field):
def get_field_display(self, field: ContactField) -> str:
"""
Returns the display value for the passed in field, or empty string if None
"""
Expand Down
10 changes: 4 additions & 6 deletions temba/contacts/templatetags/contacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,14 @@


@register.simple_tag()
def contact_field(contact, key):
field = contact.org.fields.filter(is_active=True, key=key).first()
if field is None:
return MISSING_VALUE

def contact_field(contact, field):
Copy link
Member Author

@rowanseymour rowanseymour Oct 25, 2024

Choose a reason for hiding this comment

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

we only use this in one place and we have the field object so no reason to pass key and lookup the field again.. especially since this is called for each contact (up to 50) for each field (up to 6)

value = contact.get_field_display(field)

if value and field.value_type == ContactField.TYPE_DATETIME:
value = contact.get_field_value(field)
if value:
return mark_safe(f"<temba-date value='{value.isoformat()}' display='date'></temba-date>")
display = "timedate" if field.is_proxy else "date"
return mark_safe(f"<temba-date value='{value.isoformat()}' display='{display}'></temba-date>")

return value or MISSING_VALUE

Expand Down
52 changes: 34 additions & 18 deletions temba/contacts/templatetags/tests.py
Original file line number Diff line number Diff line change
@@ -1,49 +1,65 @@
from temba.contacts.models import ContactField
from temba.tests import TembaTest

from .contacts import format_urn, name_or_urn, urn_icon, urn_or_anon
from . import contacts as tags


class ContactsTest(TembaTest):
def test_contact_field(self):
gender = self.create_field("gender", "Gender", ContactField.TYPE_TEXT)
age = self.create_field("age", "Age", ContactField.TYPE_NUMBER)
joined = self.create_field("joined", "Joined", ContactField.TYPE_DATETIME)
last_seen_on = self.org.fields.get(key="last_seen_on")
contact = self.create_contact("Bob", fields={"age": 30, "gender": "M", "joined": "2024-01-01T00:00:00Z"})

self.assertEqual("M", tags.contact_field(contact, gender))
self.assertEqual("30", tags.contact_field(contact, age))
self.assertEqual(
"<temba-date value='2024-01-01T02:00:00+02:00' display='date'></temba-date>",
tags.contact_field(contact, joined),
)
self.assertEqual("--", tags.contact_field(contact, last_seen_on))

def test_name_or_urn(self):
contact1 = self.create_contact("", urns=[])
contact2 = self.create_contact("Ann", urns=[])
contact3 = self.create_contact("Bob", urns=["tel:+12024561111", "telegram:098761111"])
contact4 = self.create_contact("", urns=["tel:+12024562222", "telegram:098762222"])

self.assertEqual("", name_or_urn(contact1, self.org))
self.assertEqual("Ann", name_or_urn(contact2, self.org))
self.assertEqual("Bob", name_or_urn(contact3, self.org))
self.assertEqual("(202) 456-2222", name_or_urn(contact4, self.org))
self.assertEqual("", tags.name_or_urn(contact1, self.org))
self.assertEqual("Ann", tags.name_or_urn(contact2, self.org))
self.assertEqual("Bob", tags.name_or_urn(contact3, self.org))
self.assertEqual("(202) 456-2222", tags.name_or_urn(contact4, self.org))

with self.anonymous(self.org):
self.assertEqual(f"{contact1.id:010}", name_or_urn(contact1, self.org))
self.assertEqual("Ann", name_or_urn(contact2, self.org))
self.assertEqual("Bob", name_or_urn(contact3, self.org))
self.assertEqual(f"{contact4.id:010}", name_or_urn(contact4, self.org))
self.assertEqual(f"{contact1.id:010}", tags.name_or_urn(contact1, self.org))
self.assertEqual("Ann", tags.name_or_urn(contact2, self.org))
self.assertEqual("Bob", tags.name_or_urn(contact3, self.org))
self.assertEqual(f"{contact4.id:010}", tags.name_or_urn(contact4, self.org))

def test_urn_or_anon(self):
contact1 = self.create_contact("Bob", urns=[])
contact2 = self.create_contact("Uri", urns=["tel:+12024561414", "telegram:098765432"])

self.assertEqual("--", urn_or_anon(contact1, self.org))
self.assertEqual("+1 202-456-1414", urn_or_anon(contact2, self.org))
self.assertEqual("--", tags.urn_or_anon(contact1, self.org))
self.assertEqual("+1 202-456-1414", tags.urn_or_anon(contact2, self.org))

with self.anonymous(self.org):
self.assertEqual(f"{contact1.id:010}", urn_or_anon(contact1, self.org))
self.assertEqual(f"{contact2.id:010}", urn_or_anon(contact2, self.org))
self.assertEqual(f"{contact1.id:010}", tags.urn_or_anon(contact1, self.org))
self.assertEqual(f"{contact2.id:010}", tags.urn_or_anon(contact2, self.org))

def test_urn_icon(self):
contact = self.create_contact("Uri", urns=["tel:+1234567890", "telegram:098765432", "viber:346376373"])
tel_urn, tg_urn, viber_urn = contact.urns.order_by("-priority")

self.assertEqual("icon-phone", urn_icon(tel_urn))
self.assertEqual("icon-telegram", urn_icon(tg_urn))
self.assertEqual("", urn_icon(viber_urn))
self.assertEqual("icon-phone", tags.urn_icon(tel_urn))
self.assertEqual("icon-telegram", tags.urn_icon(tg_urn))
self.assertEqual("", tags.urn_icon(viber_urn))

def test_format_urn(self):
contact = self.create_contact("Uri", urns=["tel:+12024561414"])

self.assertEqual("+1 202-456-1414", format_urn(contact.get_urn(), self.org))
self.assertEqual("+1 202-456-1414", tags.format_urn(contact.get_urn(), self.org))

with self.anonymous(self.org):
self.assertEqual("••••••••", format_urn(contact.get_urn(), self.org))
self.assertEqual("••••••••", tags.format_urn(contact.get_urn(), self.org))
27 changes: 7 additions & 20 deletions temba/contacts/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
ContactURN,
)
from .tasks import squash_group_counts
from .templatetags.contacts import contact_field, msg_status_badge
from .templatetags.contacts import msg_status_badge


class ContactCRUDLTest(CRUDLTestMixin, TembaTest):
Expand All @@ -59,8 +59,8 @@ def setUp(self):
self.country = AdminBoundary.create(osm_id="171496", name="Rwanda", level=0)
AdminBoundary.create(osm_id="1708283", name="Kigali", level=1, parent=self.country)

self.create_field("age", "Age", value_type="N")
self.create_field("home", "Home", value_type="S", priority=10)
self.create_field("age", "Age", value_type="N", show_in_table=True)
self.create_field("home", "Home", value_type="S", show_in_table=True, priority=10)

# sample flows don't actually get created by org initialization during tests because there are no users at that
# point so create them explicitly here, so that we also get the sample groups
Expand Down Expand Up @@ -141,7 +141,7 @@ def test_list(self, mr_mocks):
mr_mocks.contact_search('name != ""', contacts=[])
self.create_group("No Name", query='name = ""')

with self.assertNumQueries(15):
with self.assertNumQueries(16):
Copy link
Member Author

Choose a reason for hiding this comment

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

1 extra query to fetch last_seen_on and created_on as fields... but we're now not fetching every field for every contact.. so we're saving up to 300 queries per view thank you

Copy link
Member Author

Choose a reason for hiding this comment

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

that saving isn't apparent from this test because none of the fields in the test were featured and therefore displayed

response = self.client.get(list_url)

self.assertEqual([frank, joe], list(response.context["object_list"]))
Expand All @@ -162,7 +162,9 @@ def test_list(self, mr_mocks):
self.assertEqual(response.context["search"], "age = 18")
self.assertEqual(response.context["save_dynamic_search"], True)
self.assertIsNone(response.context["search_error"])
self.assertEqual(list(response.context["contact_fields"].values_list("name", flat=True)), ["Home", "Age"])
self.assertEqual(
[f.name for f in response.context["contact_fields"]], ["Home", "Age", "Last Seen On", "Created On"]
)

mr_mocks.contact_search("age = 18", contacts=[frank], total=10020)

Expand Down Expand Up @@ -3131,21 +3133,6 @@ def test_get_or_create(self):
self.assertEqual("new_key", field7.key)
self.assertEqual("New Key", field7.name) # generated

def test_contact_templatetag(self):
ContactField.get_or_create(
self.org, self.admin, "date_joined", name="join date", value_type=ContactField.TYPE_DATETIME
)

self.set_contact_field(self.joe, "first", "Starter")
self.set_contact_field(self.joe, "date_joined", "01-01-2022 8:30")

self.assertEqual(contact_field(self.joe, "first"), "Starter")
self.assertEqual(
contact_field(self.joe, "date_joined"),
"<temba-date value='2022-01-01T08:30:00+02:00' display='date'></temba-date>",
)
self.assertEqual(contact_field(self.joe, "not_there"), "--")

def test_make_key(self):
self.assertEqual("first_name", ContactField.make_key("First Name"))
self.assertEqual("second_name", ContactField.make_key("Second Name "))
Expand Down
21 changes: 7 additions & 14 deletions temba/contacts/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ class ContactListView(SpaMixin, OrgPermsMixin, BulkActionMixin, SmartListView):
sort_field = None
sort_direction = None

search_fields = ("name",) # so that search box is displayed
search_error = None

def pre_process(self, request, *args, **kwargs):
Expand All @@ -96,13 +97,6 @@ def derive_export_url(self):
search = quote_plus(self.request.GET.get("search", ""))
return f"{reverse('contacts.contact_export')}?g={self.group.uuid}&s={search}"

def derive_refresh(self):
# smart groups that are reevaluating should refresh every 2 seconds
if self.group.is_smart and self.group.status != ContactGroup.STATUS_READY:
return 200000

return None

def get_queryset(self, **kwargs):
org = self.request.org
self.search_error = None
Expand Down Expand Up @@ -148,10 +142,16 @@ def get_queryset(self, **kwargs):

def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
org = self.request.org

# prefetch contact URNs
Contact.bulk_urn_cache_initialize(context["object_list"])

# get the first 6 featured fields as well as the last seen and created fields
featured_fields = ContactField.get_fields(org, featured=True).order_by("-priority", "id")[0:6]
proxy_fields = org.fields.filter(key__in=("last_seen_on", "created_on"), is_proxy=True).order_by("-key")
context["contact_fields"] = list(featured_fields) + list(proxy_fields)

context["search_error"] = self.search_error
context["sort_direction"] = self.sort_direction
context["sort_field"] = self.sort_field
Expand Down Expand Up @@ -541,13 +541,6 @@ def build_context_menu(self, menu):
if self.has_org_perm("contacts.contact_export"):
menu.add_modax(_("Export"), "export-contacts", self.derive_export_url(), title=_("Export Contacts"))

def get_context_data(self, *args, **kwargs):
context = super().get_context_data(*args, **kwargs)
org = self.request.org

context["contact_fields"] = ContactField.get_fields(org).order_by("-show_in_table", "-priority", "id")[0:6]
return context

class Blocked(ContextMenuMixin, ContactListView):
title = _("Blocked")
system_group = ContactGroup.TYPE_DB_BLOCKED
Expand Down
6 changes: 3 additions & 3 deletions templates/contacts/contact_archived.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% extends "contacts/contact_list.html" %}
{% load i18n %}

{% block subtitle %}
{% trans "These contacts have been removed from all groups and can be deleted permanently." %}
{% endblock subtitle %}
{% block pre-table %}
<div class="mb-4">{% trans "These contacts have been removed from all groups and can be deleted permanently." %}</div>
{% endblock pre-table %}
6 changes: 3 additions & 3 deletions templates/contacts/contact_blocked.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% extends "contacts/contact_list.html" %}
{% load i18n %}

{% block subtitle %}
{% trans "All inbound messages from these contacts are ignored. They have also been removed from all groups." %}
{% endblock subtitle %}
{% block pre-table %}
<div class="mb-4">{% trans "All inbound messages from these contacts are ignored. They have also been removed from all groups." %}</div>
{% endblock pre-table %}
6 changes: 3 additions & 3 deletions templates/contacts/contact_filter.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% extends "contacts/contact_list.html" %}
{% load smartmin i18n %}

{% block subtitle %}
{% if current_group.is_smart %}<div class="font-mono text-sm mt-2 bg-gray-200 text-gray-700 p-3 rounded-lg">{{ current_group.query }}</div>{% endif %}
{% endblock subtitle %}
{% block pre-table %}
{% if current_group.is_smart %}<div class="mb-4 font-mono text-sm mt-2 bg-gray-200 text-gray-700 p-3 rounded-lg">{{ current_group.query }}</div>{% endif %}
{% endblock pre-table %}
Loading
Loading