Release User Metadata tab #34734

esoergel · 2024-06-05T21:08:54Z

Product Description

Add some basic metadata to the user page:

Technical Summary

https://dimagi.atlassian.net/browse/USH-4586

This also adds a template filter to format timestamps in django templates, and removes and replaces an old template tag that was not as nice to use.

Feature Flag

Safety Assurance

This is already live behind a feature flag.

Safety story

Automated test coverage

QA Plan

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

Shamelessly copied from the utc_to_timezone tag, though I think this is nicer, as you don't need to get and pass timezone

The reference to this was removed in 202201b

(except for function signature)

orangejenny

Looks good, glad to see this getting released. Nice cleanup of the old time zone tag.

One other UI comment, looking at the screenshot: I think wrapping the device timestamps / device ids in a table (or two columns - doesn't need all the stripes & borders), and sorting them by timestamp, would be a notable UX improvement for not a lot of effort.

orangejenny · 2024-06-11T13:34:26Z

corehq/apps/hqwebapp/templatetags/hq_shared_tags.py

@@ -746,3 +754,14 @@ def request_has_privilege(request, privilege_name):
    from corehq import privileges
    privilege = _get_obj_from_name_or_instance(privileges, privilege_name)
    return domain_has_privilege(request.domain, privilege)
+
+
+@register.filter


This is much nicer.

orangejenny · 2024-06-11T13:38:02Z

corehq/apps/users/templates/users/edit_commcare_user.html

+              <td>{{ couch_user.last_login|to_user_time:request }}</td>
+            </tr>
+            <tr>
+              <td title="Approximate date/time of the last time this user synced with the server from CommCare">


The titles are informative, but hard to discover, how about switching them to help bubbles? Example:

commcare-hq/corehq/apps/users/templates/users/roles_and_permissions.html

Lines 71 to 79 in e7c92eb

<span class="hq-help-template"

data-placement="left"

data-title="{% trans 'Linked Configurations' %}"

data-content="

{% blocktrans %}

Linked configurations are controlled from a separate, linked project space.

<a href='https://confluence.dimagi.com/display/commcarepublic/Enterprise+Release+Management' target='_blank'>Learn more</a>.

{% endblocktrans %}">

</span>

Oops, and I forgot to translate them 🤦 - apparently I'm bad at that.

esoergel · 2024-06-11T17:56:10Z

@orangejenny thanks for the feedback - just pushed those changes and updated the screenshot in the PR description

mkangia · 2024-06-13T08:09:10Z

corehq/apps/export/views/utils.py

-        try:
-            return get_timezone_for_user(couch_user, domain)
-        except AttributeError:
-            return get_timezone_for_user(None, domain)


oh this was an exact copy of corehq.util.timezones.utils.get_timezone
👍

mkangia · 2024-06-13T08:11:36Z

corehq/apps/hqwebapp/templatetags/hq_shared_tags.py

+
+
+@register.filter
+def to_user_time(dt, request):


nit: definitely the filter is useful. Thinking if this is a good opportunity to name this better?

the function get_timezone being called does fallback to domain timezone if user is missing or still use domain's timezone if override_global_tz is set to False. So, considering the timezone used could be not that of the user, to_user_time could be misleading.
Could we say for_user_display or to_user_or_domain_timezone. I do find get_timezone could also be named better, like, get_timezone_for_user_or_domain or get_timezone_for_display but that is out of scope of this PR.

I was attempting to mirror the language used in corehq.util.timezones, which talks about datetimes as "server time", "user time", or "phone time". In that context, "server time" is UTC equivalent, but without a timezone attached, and "user time" is a datetime localized to whatever timezone is appropriate to show to the user.

the function get_timezone being called does fallback to domain timezone if user is missing or still use domain's timezone if override_global_tz is set to False

I feel like that is an implementation detail detail at a lower abstraction level. I think get_timezone answers the question of "what timezone should we use for this user on this domain"? That might be a timezone specified on the user, or on the domain, or just UTC, but the answer to that question should be the same in each part of HQ. So rather than have say the case data page use the user's timezone and the sms logs use the domain timezone, and something else use UTC, they should all use the "user time", as defined by get_timezone (and get_timezone_for_user, which it calls).

Relatedly, I actually kind of think get_timezone should take request as its only arg, since you probably want to use the domain from the active request, and it already falls back to UTC if there is no domain.

Okay.

I believe I tend to have the naming really explicit and seeing _user_time didn't sit well with me because it would at times not be the user time which in my head should be the timezone user had set to and be none or raise error if there was no timezone set and leave for the calling function to figure out what it wants to do, even if it was to fallback to domain timezone.
Though its not always feasible to put everything in the name. So I am not complaining about how its here.

I think get_timezone answers the question of "what timezone should we use for this user on this domain"?

Right. I don't prefer to have to look at the function definition to understand what it would be doing or have the name be totally ambiguous so I am forced to look into it. This is in the middle.

Relatedly, I actually kind of think get_timezone should take request as its only arg, since you probably want to use the domain from the active request, and it already falls back to UTC if there is no domain.

Ya I was unsure in what cases it needed domain to be passed other than the request.

I tend to have the naming really explicit

I've noticed you and I are often on opposite sides of the spectrum here. I find long names to hurt readability. I also think of a function's parameter names as part of its public interface, so get_timezone_from_request(request) tells me nothing that get_timezone(request) doesn't. I know it's getting it from the request, because I can see it's being passed in.

it would at times not be the user time

I disagree with this, actually - it's a question of "user" vs user. It will not always be the timezone specified on the CouchUser's domain_membership, but it will always be timezone that should be shown to the user. Every time we localize a timestamp to display to the user, we should use the same logic to determine what timezone to use. There's a quote from Clean Code I'm reminded of:

Another way to know that a function is doing more than “one thing” is if you can extract another function from it with a name that is not merely a restatement of its implementation

This functionality expresses an abstraction (what timezone should we use), and callers shouldn't need to know about the implementation. To accurately express the logic happening here it'd have to be something like get_timezone_from_user_if_global_overridden_else_domain. Assuming the name is clear enough to say what the function does, I shouldn't need to think about how it does it when calling it.

I find long names to hurt readability

same same. If a name needs to be long, the function itself probably can be split.

I also think of a function's parameter names as part of its public interface

Oh yes, Like i would not mind get_timezone(user), which I would read as get timezone for user. So I am aligned with you on this.

It will not always be the timezone specified on the CouchUser's domain_membership, but it will always be timezone that should be shown to the user

Exactly. And that distinction was what I was getting to with get_timezone_for_display to clarify that we are focused on timezone that we should show or display.

Another way to know that a function is doing more than “one thing” is if you can extract another function from it with a name that is not merely a restatement of its implementation

I like that.

This functionality expresses an abstraction (what timezone should we use), and callers shouldn't need to know about the implementation

Ya, that makes sense to me, like:

def to_user_time(dt, request): timezone = _get_timezone_to_use(...) return dt.to_timezone(timezone)

To accurately express the logic happening here it'd have to be something like get_timezone_from_user_if_global_overridden_else_domain

haha, that would be too much! I might have misspoken about what I like the naming to be.

mkangia · 2024-06-13T08:12:53Z

This is already live behind a feature flag.

Nice to see more features getting GA'ed. Just checking if this needed any documentation updates for users?

mkangia · 2024-06-13T08:14:57Z

corehq/apps/users/templates/users/edit_commcare_user.html

+                    </tr>
+                  </thead>
+                  <tbody>
+                    {% for device in couch_user.devices|dictsortreversed:"last_used" %}


Is this information limited to the request domain or is it showing devices for all domains?
Same for other metadata, is it limited to the active domain only?

This is for mobile workers only, so it's all constrained to the domain

mkangia · 2024-06-13T08:15:39Z

Jumped a little late on review. Just had one concern https://github.com/dimagi/commcare-hq/pull/34734/files#r1637787037.

esoergel · 2024-06-14T14:17:39Z

Just checking if this needed any documentation updates for users?

Good point, I'll follow up in the ticket

esoergel added 10 commits June 5, 2024 16:27

Translate and reorder support tab

845b081

Translate timestamps to user time

2fed30c

Shamelessly copied from the utc_to_timezone tag, though I think this is nicer, as you don't need to get and pass timezone

Replace references to old tempate tag with filter

6326bb0

Switch secure file transfer to the new filter

cc1a464

Remove unused partial template

6672c32

The reference to this was removed in 202201b

this implementation is identical

9f1a31c

(except for function signature)

Move form data title tag to new filter

fd39ed2

remove timezone tags entirely

5ac1380

Rebuild diffs

6c6c1d3

Release support tab as "user metadata"

25783af

esoergel added the product/all-users-all-environments Change impacts all users on all environments label Jun 5, 2024

esoergel changed the title ~~Release User Metadata tab`~~ Release User Metadata tab Jun 5, 2024

esoergel added 2 commits June 6, 2024 16:18

lint

956c6f8

Add title text to a couple items

22625b7

esoergel marked this pull request as ready for review June 7, 2024 17:33

esoergel requested review from orangejenny, mkangia, biyeun and kaapstorm as code owners June 7, 2024 17:33

orangejenny approved these changes Jun 11, 2024

View reviewed changes

esoergel added 2 commits June 11, 2024 13:02

Use help template for helptext

3b5df5a

Restyle devices table and sort by last used

546fa58

translaaaaatiooooooons

d6bbb27

orangejenny approved these changes Jun 11, 2024

View reviewed changes

esoergel merged commit 344e59b into master Jun 11, 2024
13 checks passed

esoergel deleted the es/support-toolz branch June 11, 2024 18:37

esoergel mentioned this pull request Jun 12, 2024

Fix get_timezone call on exports download page #34759

Merged

3 tasks

mkangia reviewed Jun 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release User Metadata tab #34734

Release User Metadata tab #34734

esoergel commented Jun 5, 2024 •

edited

Loading

orangejenny left a comment •

edited

Loading

orangejenny Jun 11, 2024

orangejenny Jun 11, 2024

esoergel Jun 11, 2024

esoergel commented Jun 11, 2024 •

edited

Loading

mkangia Jun 13, 2024

mkangia Jun 13, 2024

esoergel Jun 14, 2024

mkangia Jun 19, 2024

esoergel Jun 19, 2024

mkangia Jun 19, 2024

mkangia commented Jun 13, 2024

mkangia Jun 13, 2024

esoergel Jun 14, 2024

mkangia commented Jun 13, 2024

esoergel commented Jun 14, 2024

	<span class="hq-help-template"
	data-placement="left"
	data-title="{% trans 'Linked Configurations' %}"
	data-content="
	{% blocktrans %}
	Linked configurations are controlled from a separate, linked project space.
	<a href='https://confluence.dimagi.com/display/commcarepublic/Enterprise+Release+Management' target='_blank'>Learn more</a>.
	{% endblocktrans %}">
	</span>

Release User Metadata tab #34734

Release User Metadata tab #34734

Conversation

esoergel commented Jun 5, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

orangejenny left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esoergel commented Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkangia commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkangia commented Jun 13, 2024

esoergel commented Jun 14, 2024

esoergel commented Jun 5, 2024 •

edited

Loading

orangejenny left a comment •

edited

Loading

esoergel commented Jun 11, 2024 •

edited

Loading