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

Add Assigned Location Column to Application Status Report #34744

Merged
merged 20 commits into from
Jul 3, 2024
Merged

Add Assigned Location Column to Application Status Report #34744

Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
30a284a
add new loc col
zandre-eng Jun 10, 2024
de825b1
refactor help text
zandre-eng Jun 12, 2024
07b5828
add loc data to assigned loc col
zandre-eng Jun 12, 2024
c229c7a
add JS to handle toggling see more link
zandre-eng Jun 12, 2024
d60fcbd
Bootstrap 5 Migration - Rebuilt diffs
zandre-eng Jun 12, 2024
362e684
move up col index to account for new col
zandre-eng Jun 13, 2024
561afac
move var to helper func
zandre-eng Jun 17, 2024
78bb53d
make primary bold even if only 1 loc
zandre-eng Jun 17, 2024
7ebeb72
do single DB query for loc names
zandre-eng Jun 17, 2024
c1a088b
constrain assigned locations to domain
zandre-eng Jun 18, 2024
37c70b4
refactor help text
zandre-eng Jun 19, 2024
e55ec3d
insert primary location at front of list
zandre-eng Jun 19, 2024
fd2f9be
add ability to shrink list of all locs again
zandre-eng Jun 19, 2024
ee88a55
refactor building loc HTML
zandre-eng Jun 20, 2024
d5b5728
use HTML class to toggle visibility
zandre-eng Jun 20, 2024
581bfe1
correctly fetch locations for web users
zandre-eng Jun 20, 2024
ba975bf
docstring + rename
zandre-eng Jul 1, 2024
b045a3d
update collapse string text
zandre-eng Jul 1, 2024
d6a20e8
move func + small refactor
zandre-eng Jul 2, 2024
e5c2342
rename refactor
zandre-eng Jul 2, 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
35 changes: 35 additions & 0 deletions corehq/apps/reports/standard/deployments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ def process_rows(self, users, fmt_for_export=False):
user_display_string(user.get('username', ''),
user.get('first_name', ''),
user.get('last_name', '')),
self.get_location_column(user),
_fmt_date(last_seen, fmt_for_export), _fmt_date(last_sync_date, fmt_for_export),
app_name or "---", build_version, commcare_version or '---',
num_unsent_forms if num_unsent_forms is not None else "---",
Expand Down Expand Up @@ -487,6 +488,40 @@ def _fmt_timestamp(timestamp):
result[0][1] = table
return result

def get_location_column(self, user):
assigned_loc_ids = user.get('assigned_location_ids')
if not assigned_loc_ids:
return '---'
primary_loc_id = user.get('location_id')
return self._get_formatted_assigned_location_names(primary_loc_id, assigned_loc_ids)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could make it simpler by passing the user to the function itself.
Fetching assigned_loc_ids and primary_loc_id isn't really needed in this function

def get_location_column(self, user):
    if not user.get('assigned_location_ids'):
        return '---'
    return self._get_formatted_assigned_location_names(user)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

561afac


def _get_formatted_assigned_location_names(self, primary_location_id, assigned_location_ids):
zandre-eng marked this conversation as resolved.
Show resolved Hide resolved
"""
Create an HTML formatted string of the given assigned location names.
The primary location will be highlighted in bold.
"""
locs = SQLLocation.objects.filter(location_id__in=assigned_location_ids)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need some sort of caching here. If reports loads say 100 users at once, we would be firing up 100 queries which could lead to bad performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add a quickcache here, however we will still need to do the initial 100 queries.

I'm thinking just before looping through users here, we could do an initial loop through the users to retrieve all their assigned location IDs and map this to a dict. This would look something like this:

@memoized
def user_locations_dict(self, users):
    all_loc_ids = []
    for user in users:
        all_loc_ids += user.get('assigned_location_ids')
    all_locs = SQLLocation.objects.filter(location_id__in=all_loc_ids).values_list('location_id', 'name')
    return {
        loc_id: loc_name
        for loc_id, loc_name in all_locs
    }

The above causes us to do a few extra loops, however we only need to do a single DB call to get all the location names. This dict can then be passed to the func responsible for parsing the HTML. @mkangia do you think this is a good approach or should using a quickcache be sufficient enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems good to me. One db query sounds good.

You could use a set instead of list to avoid repeated ids.

Also I learnt this recently from Ethan

return dict(SQLLocation.objects.filter(
    location_id__in=all_loc_ids
).values_list('location_id', 'name'))

will give you the dict you need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the code, this is very useful. I've implemented the necessary changes so that we only do a single DB hit to get all user locations for a page (7ebeb72). Unfortunately I couldn't add memoized or quickcache due to the fact that we are passing a list of users to the func (which is unhashable).

formatted_loc_names = []
for loc in locs:
if loc.location_id == primary_location_id and len(assigned_location_ids) > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we simply bold the location if its primary irrespective of the length of assigned locations?
It would be confusing if help_text says that primary location is in bold and then for one row its not in bold because its the only location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I thought it might be unnecessary to highlight the location if there is only one, but you make a good point that we should rather keep this consistent given what the help text says. I'll remove the length check from the conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

78bb53d

formatted_loc_names.append(
f'<strong>{loc.name}</strong>'
)
else:
formatted_loc_names.append(loc.name)

formatted_str = ', '.join(formatted_loc_names[:4])
all_str = ', '.join(formatted_loc_names)
eplise_str = _('...See more') if len(formatted_loc_names) > 4 else ''
out_str = ('''
<div>
<span class="locations-list">{}</span>
<a href="#" class="toggle-all-locations">{}</a>
<span class="all-locations-list" style="display:none">{}</span>
</div>
''').format(formatted_str, eplise_str, all_str)
return format_html(out_str)


def _get_commcare_version(app_version_info):
commcare_version = (
Expand Down