-
-
Notifications
You must be signed in to change notification settings - Fork 255
Add frontend for digest settings #3344
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.
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 frontend for digest settings #3344
Conversation
66fd15f
to
eb6119b
Compare
<details open={notificationsEnabled}> | ||
<summary | ||
className="mt-4" | ||
onClick={(e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this because clicking on the summary would toggle the details's state. (Same thing happens in BrainzPlayer settings.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ready for review?
A comment in the meantime.
Yes @MonkeyDo , its ready for review. Also about mayhem's comment on the previous PR, where do you think we should show this information? |
I rephrased it a bit and suggest the simplest solution of them all: an HTML paragraph ( We will always email you right away about <b>important account issues</b> (like problems with connected services).<br/>
For everything else, you can choose to turn off notifications or receive them in a periodic digest. ![]() And keeping the alert-info to catch user's eyes if they disable email notifications: |
@MonkeyDo, Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UI is looking good! A couple of changes for some edge cases, and some compliance to the codebase style that will simplify a couple of things.
React.useEffect(() => { | ||
async function fetchDigestSettings() { | ||
try { | ||
const response = await fetch("/settings/digest-setting/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is correct and works, how we usually do this initial data fetching is with a react-router RouteLoader utility that automatically hits a POST endpoint on the server of the same name as the page. See for example:
listenbrainz-server/frontend/js/src/settings/routes/index.tsx
Lines 34 to 42 in c882a95
path: "music-services/details/", | |
lazy: { | |
Component: async () => { | |
return (await import("../music-services/details/MusicServices")).default; | |
}, | |
loader: async () => { | |
return RouteLoader; | |
}, | |
}, |
And that way, you can use the initial data with a hook, and as a bonus you won't need to store the 'initial...' values in the state as they will be present in the loaderData for you to compare with:
listenbrainz-server/frontend/js/src/settings/music-services/details/MusicServices.tsx
Lines 44 to 50 in c882a95
const loaderData = useLoaderData() as MusicServicesLoaderData; | |
const { appleAuth } = React.useContext(GlobalAppContext); | |
const [permissions, setPermissions] = React.useState({ | |
spotify: loaderData.current_spotify_permissions, | |
critiquebrainz: loaderData.current_critiquebrainz_permissions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the server side of this example for reference:
listenbrainz-server/listenbrainz/webserver/views/settings.py
Lines 192 to 254 in 1235c47
@settings_bp.post('/music-services/details/') | |
@api_login_required | |
def music_services_details(): | |
spotify_service = SpotifyService() | |
spotify_user = spotify_service.get_user(current_user.id) | |
if spotify_user: | |
permissions = set(spotify_user["scopes"]) | |
if permissions == SPOTIFY_IMPORT_PERMISSIONS: | |
current_spotify_permissions = "import" | |
elif permissions == SPOTIFY_LISTEN_PERMISSIONS: | |
current_spotify_permissions = "listen" | |
else: | |
current_spotify_permissions = "both" | |
else: | |
current_spotify_permissions = "disable" | |
critiquebrainz_service = CritiqueBrainzService() | |
critiquebrainz_user = critiquebrainz_service.get_user(current_user.id) | |
current_critiquebrainz_permissions = "review" if critiquebrainz_user else "disable" | |
soundcloud_service = SoundCloudService() | |
soundcloud_user = soundcloud_service.get_user(current_user.id) | |
current_soundcloud_permissions = "listen" if soundcloud_user else "disable" | |
apple_service = AppleService() | |
apple_user = apple_service.get_user(current_user.id) | |
current_apple_permissions = "listen" if apple_user and apple_user["refresh_token"] else "disable" | |
lastfm_service = LastfmService() | |
lastfm_user = lastfm_service.get_user(current_user.id) | |
current_lastfm_permissions = "import" if lastfm_user else "disable" | |
funkwhale_tokens = db_funkwhale.get_all_user_tokens(db_conn, current_user.id) | |
funkwhale_host_urls = [token["host_url"] for token in funkwhale_tokens] | |
current_funkwhale_permission = "listen" if funkwhale_tokens else "disable" | |
librefm_service = LibrefmService() | |
librefm_user = librefm_service.get_user(current_user.id) | |
current_librefm_permissions = "import" if librefm_user else "disable" | |
data: dict[str, Any] = { | |
"current_spotify_permissions": current_spotify_permissions, | |
"current_critiquebrainz_permissions": current_critiquebrainz_permissions, | |
"current_soundcloud_permissions": current_soundcloud_permissions, | |
"current_apple_permissions": current_apple_permissions, | |
"current_lastfm_permissions": current_lastfm_permissions, | |
"current_funkwhale_permission": current_funkwhale_permission, | |
"funkwhale_host_urls": funkwhale_host_urls, | |
"current_librefm_permissions": current_librefm_permissions, | |
} | |
if lastfm_user: | |
data["current_lastfm_settings"] = { | |
"external_user_id": lastfm_user["external_user_id"], | |
"latest_listened_at": lastfm_user["latest_listened_at"], | |
} | |
if librefm_user: | |
data["current_librefm_settings"] = { | |
"external_user_id": librefm_user["external_user_id"], | |
"latest_listened_at": librefm_user["latest_listened_at"], | |
} | |
return jsonify(data) |
// eslint-disable-next-line no-console | ||
console.error("Could not update notification settings.", error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't want to log this to console, and instead show the error to the user in the toast message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we show the complete error in a toast message or show the abstracted version of error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I think it would be useful to have more details than just 'something went wrong' so users can report issues they encounter. So error.toString()
might give you a good text version, but you might need to try to trigger an error and see the result.
EDIT: Just seeing that you throw the error above with the statuscode. I think that would be a good start.
Might be good to try and get an errormessage from the API reponse, but not sure what shape that will take: if the API returns a json object with the message in it, you'll need to await response.json()
. Otherwise probably await response.text()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API responds with a json object. I'll throw the response.error instead
if (loading) { | ||
return <div>Loading notification settings...</div>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If using the react-router RouteLoader, this won't be necessary anymore.
lazy: { | ||
Component: async () => { | ||
return (await import("../notification-settings/NotificationSettings")).default; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the react-query loader would be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think this PR is ready, save for this final small change.
However I think two things are going to be required to merge everything clearly, but i need your input on this to confirm or infirm because I'm a bit lost :P
- There is front-end code in #3345 that needs to be removed, as it will be replaced by the code in this PR
- The back-end needs a new POST endpoint that will return the user's notifications settings saved in the database, at POST
/settings/notifications/
(whatfetchDigestSettings
used to do). Currently the queryloader automatically tries to hit that endpoint, which results in an error:

I'll remove front end bits from #3345.
Yes, I was waiting for this PR to be approved so I could update the endpoint names accordingly. |
OK, thanks for the update! EDIT: I see you are already on that 👍 |
@MonkeyDo Done. |
Yes indeed you are right, and we know it looks wrong.
And example of that here: listenbrainz-server/listenbrainz/webserver/views/donors.py Lines 15 to 22 in a1fe029
|
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
I am hitting an error when I try to load the page, something to do with Oauth grant type.
|
The |
Makes sense! |
It would be possible, but then we would have to have a separate endpoint and hardcode that endpoint for each route, which we wanted to avoid. |
Thanks, I hardcoded the return value, I'm only looking at checking the UI one last time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! 🚀
Thanks for your patience :)
2f065ce
into
metabrainz:metabrainz-notifications
Ohh, got it! Thanks for the detailed explanation. |
Added a Notification Settings page according to the feedback by @MonkeyDo in #3345.
EDIT: UI mockups