-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5587 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 551 551
Lines 25667 25663 -4
=========================================
- Hits 25667 25663 -4 ☔ View full report in Codecov by Sentry. |
if field is None: | ||
return MISSING_VALUE | ||
|
||
def contact_field(contact, field): |
There was a problem hiding this comment.
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)
@@ -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): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@@ -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): |
There was a problem hiding this comment.
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
font-size: 0.75rem; | ||
} | ||
|
||
.span9 .contact_list tr.contacts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah bootstrap
<temba-modax header="{% trans "Create Group" %}" | ||
endpoint="{% url 'contacts.contactgroup_create' %}" | ||
-temba-loaded="handleCreateGroupModalLoaded" | ||
id="create-group-modal"> | ||
</temba-modax> | ||
<div class="mt-4 shadow rounded-lg rounded-bl-none rounded-br-none bg-white"> | ||
{% include "includes/short_pagination.html" %} | ||
{% if paginator.is_es_search and not page_obj.has_next_page and page_obj.number == paginator.num_pages and paginator.count > 10000 %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_es_search
is never set so this never appears... but I don't think we need this.. nobody is (should be) paging thru more than 200 pages of contacts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one can use the URL to jump to the any page number
Saves up to 299 DB queries per request 🍻
Simplifies a bunch of template logic by including last_seen_on and created_on as proxy contact fields.