Community Health: monthly community health report system - #4336
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
96979a9 to
e6cf8c9
Compare
mathjazz
left a comment
There was a problem hiding this comment.
Nice work!
Some high-level questions:
- Why is the /insights page shown to and the notification sent to
is_staffusers , but the emails can be opted in byis_superuserusers? - Why don't we automatically opt in all
is_staffusers also for receiving emails and drop the settings? This is an altert system afterall. Or at least opt them in by default, if we really want to keep the ability to opt out. - Have we tested the notification in the translate view, which has its own notification implementation?
- The docstring says rows are "ranked by the size of that change", but the biggest gains come first and the worst regressions land at the bottom of the table. In an alert system, the regressions are the actionable half. Sort by
abs(percentage)to match the docstring? - The new command needs to be documented on
documentation/docs/dev/deployment.md. We should mention it silently depends on cron ordering - it reads the current month's snapshot — whichcollect_chs_snapshotscreates on that same day. - We should also document the reporting feature for the Admins. Note that there's still #4338 (comment).
|
|
||
| {% if description and (description.startswith("Unreviewed suggestions") or notification.verb == "has reviewed suggestions" or notification.verb == "ignore") %} | ||
| <span class="description">{{ description|safe }}</span> | ||
| <div class="description">{{ description|safe }}</div> |
There was a problem hiding this comment.
What's the purpose of this change?
There was a problem hiding this comment.
If we keep it as a span, if the description contains html elements, they will all be converted to text, which is not what we want.
There was a problem hiding this comment.
The main point is now we have a system notification that automatically sends but also contains more complex html + css, which was not supported with the current code. All our previous system notifications were just one-liner spans.
There was a problem hiding this comment.
Could we render this content under the existing .message class rather than restyling .description? The heading, paragraph, list and table styling already exists for .message — we'd keep the div, since block elements can't live in a span, but UserNotification.css wouldn't need the new block, and style.css wouldn't need the merged selectors either.
One consequence to weigh: the unreviewed-suggestions notification renders through the same component, so it would also pick up .message's padding and heading styling. Probably an improvement, but worth a look before and after.
There was a problem hiding this comment.
What's the purpose of these changes?
There was a problem hiding this comment.
Because I changed description to a div, I also want it to inherit css styling as well.
| assert mail.outbox[0].to == [user_a.contact_email] | ||
|
|
||
|
|
||
| @pytest.mark.django_db |
There was a problem hiding this comment.
We should also add a test for the case when none of the locales passes a threshold.
I don't think all superusers are PMs interested in this information, so they should have a way to opt out. I have the feeling the majority falls into this category, hence the opt-in suggestion to Jamie. |
I actually think it should be only reserved to is_staff users - I made the change before you reviewed, but it seems I didn't completely change options.
Just like the other email systems in Pontoon, I think we should keep the option to opt in given both the notification & email output very similar things.
Sure. Implemented.
Sure, I'll make the changes in this PR. Implemented. |
5b5bde8 to
b9a755c
Compare
|
I've been testing out the 2% threshold for CHS, and I'm beginning to think that is not sufficient, as some Common Voice locales may have deltas greater than 2% but these changes an insignificantly small. Perhaps changing to a 2 point threshold makes more sense? |
OK. We should at least file an issue then to introduce a "PM" user role. |
mathjazz
left a comment
There was a problem hiding this comment.
Thanks for the update!
Could you please unify the copy between the notification and the email and update the screenshot with the latest version?
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
How did we get to 2% in the first place? Have we looked at the old data? |
We didn't really discuss it, this was something I more or less added as a placeholder. |
mathjazz
left a comment
There was a problem hiding this comment.
Left some notes to make the copy a bit more condensed.
…port.html Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
…port.html Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
…port.html Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
…port.html Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
mathjazz
left a comment
There was a problem hiding this comment.
Sent copy through LLM and left some suggestions.
The rest looks good, great work!
| information on locale growth & decline in easily ingestible formats. | ||
|
|
||
| ## Insights dashboard | ||
|
|
There was a problem hiding this comment.
I would drop this section, which would allow us to keep the existing heading levels. 4 levels seems like a lot for a relatively short page.
|
|
||
| A team is included when it is enabled for at least one [key project](#key-projects), has a snapshot in both months, and its score changed by at least 2%. The threshold can be changed through the `MONTHLY_HEALTH_REPORT_CHS_THRESHOLD` environment variable. | ||
|
|
||
| Staff users receive the report as a notification in Pontoon. To also receive it by email, enable `Monthly locale health report` in the `Email` section of your [settings](../localizer/users.md). |
There was a problem hiding this comment.
We link to the Settings page, but the new setting is not documented there.
|
|
||
| {% if description and (description.startswith("Unreviewed suggestions") or notification.verb == "has reviewed suggestions" or notification.verb == "ignore") %} | ||
| <span class="description">{{ description|safe }}</span> | ||
| <div class="description">{{ description|safe }}</div> |
There was a problem hiding this comment.
Could we render this content under the existing .message class rather than restyling .description? The heading, paragraph, list and table styling already exists for .message — we'd keep the div, since block elements can't live in a span, but UserNotification.css wouldn't need the new block, and style.css wouldn't need the merged selectors either.
One consequence to weigh: the unreviewed-suggestions notification renders through the same component, so it would also pick up .message's padding and heading styling. Probably an improvement, but worth a look before and after.
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
…rt_notifications_and_emails.py Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
This PR creates a community health alert system for PMs, and allows PMs see month-over-month changes to CHS at a glance. It is compiled monthly.
Fixes #4329.