Add endpoints for digest settings. #3345
Conversation
fettuccinae
changed the title
|
@mayhem -- PR ready for review. |
|
@MonkeyDo can you please have a look at the react bits? |
| response.raise_for_status() | ||
|
|
||
|
|
||
| def get_digest_preference(musicbrainz_row_id: int) -> dict: |
There was a problem hiding this comment.
Oops, sorry. Got lost in branches. Added it now.
There was a problem hiding this comment.
Overall I think we need to think about other (future) options and settings for notifications rather than just adding this one input.
I envision a separate settings sub-page (see screenshot below, left column), which for now can contain only these three things: a switch for email notifications, a switch for digest emails and the input for the number of days.
I threw this mockup together, using the "Music Player" settings page (source) markup as the base:
- The first switch should say "Enable e-mail notifications" with an 's' at the end
- The save button is missing in this screenshot but should be like the second screenshot
The digest section automatically collapses when emails are disabled (using the <summary> and <details> html elements).
Functionally, disabling emails should also disable digest once you click save (I understand that we might not have an endpoint yet to disable emails entirely but we'll be set up for the future).
We should also show an alert (css classes: alert alert-info) when emails are disabled:
| <label htmlFor="digest-age" className="form-label"> | ||
| Digest age (in days) | ||
| </label> | ||
| <input |
There was a problem hiding this comment.
How do you like the number of days input inline, like I did in my mockup?
I used this markup if it is useful:
<div class="mb-4">Send digest emails every <input type="number" class="d-inline form-control" id="digest-age" min="1" max="100" style="max-width: 4em;"> days</div>There was a problem hiding this comment.
I prefer your version, its more informative.
| className="form-control" | ||
| id="digest-age" | ||
| min={1} | ||
| max={100} |
There was a problem hiding this comment.
I wonder if that is an acceptable upper limit.
100 days seems like a lot for a digest...
There was a problem hiding this comment.
We currently have 100 set as the upper limit in the endpoint. But yes, its a lot. What do you think would be a reasonable upper limit?
| id="digest-age" | ||
| min={1} | ||
| max={100} | ||
| value={digestAge ?? ""} |
There was a problem hiding this comment.
What should be the default value?
There was a problem hiding this comment.
We can pass it as None through the endpoint, MeB server will default it to 7 days.
|
@MonkeyDo I like your mockup better. I'll implement a separate notification sub-settings page. Should I make another PR for it since this one will get big, and remove the react bits from this one? |
I think it is fine to merge the PR as is and do those changes in another PR. |
There was a problem hiding this comment.
OK by me, but can we please add a little bit of more text explaining what the user options are?
"ListenBrainz may need to contact you about important events about your ListenBrainz account (e.g. problems with connected services) or about events that are happening on ListenBrainz. We will always send the important emails immediately, but all other notifications can be turned off, or sent via a digest periodically:"
Added
/digest-settingendpoints and their respective methods to interact with MeB.org digest preference endpoints.Removed unused imports.