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

Release User Metadata tab #34734

Merged
merged 15 commits into from
Jun 11, 2024
Merged

Release User Metadata tab #34734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
845b081
Translate and reorder support tab
esoergel May 23, 2024
2fed30c
Translate timestamps to user time
esoergel May 23, 2024
6326bb0
Replace references to old tempate tag with filter
esoergel May 23, 2024
cc1a464
Switch secure file transfer to the new filter
esoergel May 24, 2024
6672c32
Remove unused partial template
esoergel May 24, 2024
9f1a31c
this implementation is identical
esoergel May 24, 2024
fd39ed2
Move form data title tag to new filter
esoergel May 24, 2024
5ac1380
remove timezone tags entirely
esoergel May 24, 2024
6c6c1d3
Rebuild diffs
esoergel May 24, 2024
25783af
Release support tab as "user metadata"
esoergel Jun 5, 2024
956c6f8
lint
esoergel Jun 6, 2024
22625b7
Add title text to a couple items
esoergel Jun 7, 2024
3b5df5a
Use help template for helptext
esoergel Jun 11, 2024
546fa58
Restyle devices table and sort by last used
esoergel Jun 11, 2024
d6bbb27
translaaaaatiooooooons
esoergel Jun 11, 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
1 change: 0 additions & 1 deletion corehq/apps/app_manager/templates/app_manager/apps_base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
{% load xforms_extras %}
{% load url_extras %}
{% load hq_shared_tags %}
{% load timezone_tags %}
{% load i18n %}
{% load compress %}

Expand Down
2 changes: 0 additions & 2 deletions corehq/apps/app_manager/templates/app_manager/partials/releases/releases.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
{% load xforms_extras %}
{% load url_extras %}
{% load timezone_tags %}
{% load hq_shared_tags %}
{% load timezone_tags %}
{% load i18n %}
{% load crispy_forms_tags %}

Expand Down
3 changes: 1 addition & 2 deletions corehq/apps/export/templates/export/download_data_files.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{% extends "hqwebapp/bootstrap3/base_section.html" %}
{% load hq_shared_tags %}
{% load timezone_tags %}
{% load i18n %}
{% load compress %}

Expand Down Expand Up @@ -33,7 +32,7 @@ <h2>{% trans "Download Files" %}</h2>
<textarea id="url_{{ data_file.id }}" style="display: none">{{ url_base }}{% url 'download_data_file' domain data_file.id data_file.filename %}</textarea>
</td>
<td>
{% trans "Available until" %}: {% utc_to_timezone data_file.delete_after timezone %}
{% trans "Available until" %}: {{ data_file.delete_after|to_user_time:request }}
</td>
<td style="width: 6em; text-align: right;">
<a href="#"
Expand Down
16 changes: 2 additions & 14 deletions corehq/apps/export/views/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from django.utils.translation import gettext_lazy
from django.views.generic import View

import pytz
from memoized import memoized

from dimagi.utils.web import get_url_base, json_response
Expand Down Expand Up @@ -55,21 +54,11 @@
from corehq.blobs.exceptions import NotFound
from corehq.privileges import DAILY_SAVED_EXPORT, EXCEL_DASHBOARD
from corehq.util.download import get_download_response
from corehq.util.timezones.utils import get_timezone_for_user
from corehq.util.timezones.utils import get_timezone
from corehq.apps.reports.analytics.esaccessors import get_case_types_for_domain
from corehq.apps.app_manager.dbaccessors import get_app_ids_in_domain


def get_timezone(domain, couch_user):
if not domain:
return pytz.utc
else:
try:
return get_timezone_for_user(couch_user, domain)
except AttributeError:
return get_timezone_for_user(None, domain)
Copy link
Contributor

Choose a reason for hiding this comment

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

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



def user_can_view_deid_exports(domain, couch_user):
return (domain_has_privilege(domain, privileges.DEIDENTIFIED_DATA)
and has_permission_to_view_report(couch_user, domain, DEID_EXPORT_PERMISSION))
Expand Down Expand Up @@ -162,7 +151,7 @@ def create_new_export_instance(self, schema, username, export_settings=None):
)
instance.is_daily_saved_export = True

span = datespan_from_beginning(self.domain_object, get_timezone(self.domain, self.request.couch_user))
span = datespan_from_beginning(self.domain_object, get_timezone(self.request, self.domain))
instance.filters.date_period = DatePeriod(
period_type="since", begin=span.startdate.date()
)
Expand Down Expand Up @@ -335,7 +324,6 @@ def dispatch(self, request, *args, **kwargs):
def get_context_data(self, **kwargs):
context = super(DataFileDownloadList, self).get_context_data(**kwargs)
context.update({
'timezone': get_timezone_for_user(self.request.couch_user, self.domain),
'data_files': DataFile.get_all(self.domain),
'is_admin': can_upload_data_files(self.domain, self.request.couch_user),
'url_base': get_url_base(),
Expand Down
23 changes: 21 additions & 2 deletions corehq/apps/hqwebapp/templatetags/hq_shared_tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,14 @@
from dimagi.utils.web import json_handler

from corehq import privileges
from corehq.apps.hqwebapp.exceptions import AlreadyRenderedException, TemplateTagJSONException
from corehq.apps.hqwebapp.exceptions import (
AlreadyRenderedException,
TemplateTagJSONException,
)
from corehq.apps.hqwebapp.models import Alert
from corehq.motech.utils import pformat_json
from corehq.util.timezones.conversions import ServerTime
from corehq.util.timezones.utils import get_timezone

register = template.Library()

Expand Down Expand Up @@ -114,7 +119,10 @@ def domains_for_user(context, request, selected_domain=None):
the user doc updates via save.
"""

from corehq.apps.domain.views.base import get_domain_links_for_dropdown, get_enterprise_links_for_dropdown
from corehq.apps.domain.views.base import (
get_domain_links_for_dropdown,
get_enterprise_links_for_dropdown,
)
domain_links = get_domain_links_for_dropdown(request.couch_user)

# Enterprise permissions projects aren't in the dropdown, but show a hint they exist
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much nicer.

def to_user_time(dt, request):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"""Convert a datetime to a readable string in the user's timezone"""
if not dt:
return "---"
if not isinstance(dt, datetime):
raise ValueError("to_user_time only accepts datetimes")
timezone = get_timezone(request, getattr(request, 'domain', None))
return ServerTime(dt).user_time(timezone).ui_string()
11 changes: 0 additions & 11 deletions ...hqwebapp/tests/data/bootstrap5_diffs/reports/form/partials/form_breadcrumbs.html.diff.txt
View file
Open in desktop

This file was deleted.

26 changes: 13 additions & 13 deletions corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/reports/reportdata/case_data.html.diff.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
{% load case_tags %}
{% load hq_shared_tags %}
{% load i18n %}
@@ -9,7 +9,7 @@
@@ -8,7 +8,7 @@
<link rel="stylesheet" type="text/css" href="{% static "hqwebapp/css/proptable.css" %}">
{% endblock %}

Expand All @@ -15,7 +15,7 @@

{% block page_content %}

@@ -30,7 +30,7 @@
@@ -29,7 +29,7 @@
<div id="report-content">
<div id="tabbed-content-container">
<div class="row">
Expand All @@ -24,7 +24,7 @@
{% if is_case_type_deprecated %}
<div class="alert alert-warning">
<p>
@@ -47,25 +47,25 @@
@@ -46,25 +46,25 @@
</div>
</div>
<div class="row">
Expand Down Expand Up @@ -58,7 +58,7 @@
{% endif %}
</ul>

@@ -74,18 +74,18 @@
@@ -73,18 +73,18 @@
<div class="row-fluid">
{% if case_property_tables %}
{% if show_expand_collapse_buttons %}
Expand All @@ -84,7 +84,7 @@
data-parent="#property-table-{{ forloop.counter }}-parent"
href="#property-table-{{ forloop.counter }}"
class="collapse in">
@@ -96,7 +96,7 @@
@@ -95,7 +95,7 @@
</div>
{% endif %}
<div class="panel-collapse collapse in" id="property-table-{{ forloop.counter }}">
Expand All @@ -93,7 +93,7 @@
{% include "reports/partials/case_property_table.html" with rows=table.rows %}
</div>
</div>
@@ -104,11 +104,11 @@
@@ -103,11 +103,11 @@
{% endfor %}
</div>
{% else %}
Expand All @@ -108,7 +108,7 @@
</a>
{% blocktrans %}
<p>
@@ -145,7 +145,7 @@
@@ -144,7 +144,7 @@
{% if ledgers %}
<div class="tab-pane" id="case-ledgers">
{% if show_transaction_export %}
Expand All @@ -117,7 +117,7 @@
{% endif %}
{% for section_id, product_map in ledgers.items %}
<h1>{% blocktrans %}Section: {{ section_id }}{% endblocktrans %}</h1>
@@ -174,7 +174,7 @@
@@ -173,7 +173,7 @@
{% endif %}

<div class="tab-pane row" id="history">
Expand All @@ -126,7 +126,7 @@
<ul data-bind="foreach: $root.form_type_facets">
<li>
<strong><span data-bind="text: form_name"></span></strong>:
@@ -188,10 +188,10 @@
@@ -187,10 +187,10 @@
<table class="table table-striped datatable table-hover">
<thead>
<tr>
Expand All @@ -141,7 +141,7 @@
</tr>
</thead>
<tbody data-bind="foreach: xforms">
@@ -206,7 +206,7 @@
@@ -205,7 +205,7 @@
</td>
<td>
<span data-bind="text: user_type"></span>
Expand All @@ -150,7 +150,7 @@
</td>
</tr>
</tbody>
@@ -215,26 +215,26 @@
@@ -214,26 +214,26 @@
<div class="dataTables_info">
<span data-bind="text: $root.page_start_num() + '-' + $root.page_end_num() + ' / ' + $root.total_rows()"></span>
</div>
Expand Down Expand Up @@ -183,7 +183,7 @@
<div id="xform_data_panel"></div>
</div>
</div> <!-- end case-history tab -->
@@ -250,36 +250,36 @@
@@ -249,36 +249,36 @@

{% if can_edit_data %}
<div id="case-actions" class="clearfix form-actions">
Expand Down Expand Up @@ -229,7 +229,7 @@
<i class="fa fa-archive"></i>
{% trans 'Close Case' %}
</button>
@@ -293,6 +293,6 @@
@@ -292,6 +292,6 @@

{% block modals %}{{ block.super }}
{% if show_properties_edit %}
Expand Down
8 changes: 4 additions & 4 deletions corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/reports/reportdata/form_data.html.diff.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@
-{% extends "hqwebapp/bootstrap3/base_section.html" %}
+{% extends "hqwebapp/bootstrap5/base_section.html" %}
{% load case_tags %}
{% load timezone_tags %}
{% load hq_shared_tags %}
@@ -8,7 +8,7 @@
{% load i18n %}
@@ -7,7 +7,7 @@
<link rel="stylesheet" type="text/css" href="{% static "hqwebapp/css/proptable.css" %}">
{% endblock %}

-{% requirejs_main 'reports/js/bootstrap3/form_data_main' %}
+{% requirejs_main_b5 'reports/js/bootstrap5/form_data_main' %}

{% block title %}Form: {{ form_name }}{% if form_received_on %} ({% utc_to_timezone form_received_on timezone %}){% endif %}{% endblock %}
{% block title %}Form: {{ form_name }} {% if form_received_on %} ({{ form_received_on|to_user_time:request }}){% endif %}{% endblock %}

@@ -26,5 +26,5 @@
@@ -25,5 +25,5 @@
<strong>{{ instance.problem }}</strong>
</div>
{% endif %}
Expand Down
19 changes: 0 additions & 19 deletions corehq/apps/reports/templates/reports/form/partials/bootstrap3/form_breadcrumbs.html
View file
Open in desktop

This file was deleted.

19 changes: 0 additions & 19 deletions corehq/apps/reports/templates/reports/form/partials/bootstrap5/form_breadcrumbs.html
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion corehq/apps/reports/templates/reports/reportdata/bootstrap3/case_data.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
{% load hq_shared_tags %}
{% load i18n %}
{% load proptable_tags %}
{% load timezone_tags %}

{% block head %} {{ block.super }}
<link rel="stylesheet" type="text/css" href="{% static "hqwebapp/css/proptable.css" %}">
Expand Down
3 changes: 1 addition & 2 deletions corehq/apps/reports/templates/reports/reportdata/bootstrap3/form_data.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{% extends "hqwebapp/bootstrap3/base_section.html" %}
{% load case_tags %}
{% load timezone_tags %}
{% load hq_shared_tags %}
{% load i18n %}

Expand All @@ -10,7 +9,7 @@

{% requirejs_main 'reports/js/bootstrap3/form_data_main' %}

{% block title %}Form: {{ form_name }}{% if form_received_on %} ({% utc_to_timezone form_received_on timezone %}){% endif %}{% endblock %}
{% block title %}Form: {{ form_name }} {% if form_received_on %} ({{ form_received_on|to_user_time:request }}){% endif %}{% endblock %}

{% block page_content %}
{% initial_page_data 'ordered_question_values' ordered_question_values %}
Expand Down
1 change: 0 additions & 1 deletion corehq/apps/reports/templates/reports/reportdata/bootstrap5/case_data.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
{% load hq_shared_tags %}
{% load i18n %}
{% load proptable_tags %}
{% load timezone_tags %}

{% block head %} {{ block.super }}
<link rel="stylesheet" type="text/css" href="{% static "hqwebapp/css/proptable.css" %}">
Expand Down
3 changes: 1 addition & 2 deletions corehq/apps/reports/templates/reports/reportdata/bootstrap5/form_data.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{% extends "hqwebapp/bootstrap5/base_section.html" %}
{% load case_tags %}
{% load timezone_tags %}
{% load hq_shared_tags %}
{% load i18n %}

Expand All @@ -10,7 +9,7 @@

{% requirejs_main_b5 'reports/js/bootstrap5/form_data_main' %}

{% block title %}Form: {{ form_name }}{% if form_received_on %} ({% utc_to_timezone form_received_on timezone %}){% endif %}{% endblock %}
{% block title %}Form: {{ form_name }} {% if form_received_on %} ({{ form_received_on|to_user_time:request }}){% endif %}{% endblock %}

{% block page_content %}
{% initial_page_data 'ordered_question_values' ordered_question_values %}
Expand Down
18 changes: 0 additions & 18 deletions corehq/apps/reports/templatetags/timezone_tags.py
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion corehq/apps/sms/templates/sms/compose.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{% extends 'hqwebapp/bootstrap3/base_section.html' %}
{% load i18n %}
{% load hq_shared_tags %}
{% load timezone_tags %}
{% load crispy_forms_tags %}

{% requirejs_main "sms/js/compose" %}
Expand Down
1 change: 0 additions & 1 deletion corehq/apps/sms/templates/sms/subscribe_sms.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
{% load i18n %}
{% load crispy_forms_tags %}
{% load hq_shared_tags %}
{% load timezone_tags %}

{% block page_content %}
{% crispy form %}
Expand Down
Loading
Loading