Skip to content

Conversation

fettuccinae
Copy link
Contributor

@fettuccinae fettuccinae commented Aug 15, 2025

Added a Notification Settings page according to the feedback by @MonkeyDo in #3345.
EDIT: UI mockups

Screenshot 2025-08-29 173917 Screenshot 2025-08-29 173932

@fettuccinae fettuccinae force-pushed the frontend branch 2 times, most recently from 66fd15f to eb6119b Compare August 19, 2025 10:10
<details open={notificationsEnabled}>
<summary
className="mt-4"
onClick={(e) => {
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 added this because clicking on the summary would toggle the details's state. (Same thing happens in BrainzPlayer settings.)

@fettuccinae
Copy link
Contributor Author

@MonkeyDo

Copy link
Member

@MonkeyDo MonkeyDo left a 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.

@fettuccinae
Copy link
Contributor Author

fettuccinae commented Aug 26, 2025

Is this ready for review?

Yes @MonkeyDo , its ready for review. Also about mayhem's comment on the previous PR, where do you think we should show this information?

@MonkeyDo
Copy link
Member

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 (<p>) with the text:

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.
image

And keeping the alert-info to catch user's eyes if they disable email notifications:
image

@fettuccinae
Copy link
Contributor Author

fettuccinae commented Aug 27, 2025

@MonkeyDo, Done.
I have binded notificationEnabled and digestEnabled booleans, because there is no endpoint for us to set notification options rn, and if a user have their digest disabled, we only send important notifications(for now).

@fettuccinae fettuccinae requested a review from MonkeyDo August 27, 2025 13:01
Copy link
Member

@MonkeyDo MonkeyDo left a 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/");
Copy link
Member

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:

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:

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,

Copy link
Member

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:

@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)

Comment on lines 61 to 62
// eslint-disable-next-line no-console
console.error("Could not update notification settings.", error);
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@MonkeyDo MonkeyDo Aug 27, 2025

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()

Copy link
Contributor Author

@fettuccinae fettuccinae Aug 27, 2025

Choose a reason for hiding this comment

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

Comment on lines 72 to 74
if (loading) {
return <div>Loading notification settings...</div>;
}
Copy link
Member

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;
},
Copy link
Member

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.

@fettuccinae fettuccinae requested a review from MonkeyDo August 27, 2025 17:59
Copy link
Member

@MonkeyDo MonkeyDo left a 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

  1. There is front-end code in #3345 that needs to be removed, as it will be replaced by the code in this PR
  2. The back-end needs a new POST endpoint that will return the user's notifications settings saved in the database, at POST /settings/notifications/ (what fetchDigestSettings used to do). Currently the queryloader automatically tries to hit that endpoint, which results in an error:
Image

@fettuccinae
Copy link
Contributor Author

fettuccinae commented Sep 5, 2025

  1. There is front-end code in Add endpoints for digest settings. #3345 that needs to be removed, as it will be replaced by the code in this PR

I'll remove front end bits from #3345.

  1. The back-end needs a new POST endpoint that will return the user's notifications settings saved in the database, at POST /settings/notifications/ (what fetchDigestSettings used to do). Currently the queryloader automatically tries to hit that endpoint, which results in an error:

Yes, I was waiting for this PR to be approved so I could update the endpoint names accordingly.

@MonkeyDo
Copy link
Member

MonkeyDo commented Sep 5, 2025

Yes, I was waiting for this PR to be approved so I could update the endpoint names accordingly.

OK, thanks for the update!
For what it's worth, you will need two POST endpoints. The one that already exists in 3345 for updating the notification settings is not the same as the one you need for the routeloader

EDIT: I see you are already on that 👍

@fettuccinae
Copy link
Contributor Author

@MonkeyDo Done.
Small question: why do we use POST to fetch data instead of GET in route loader? My understanding was that to use GET to read and POST to write .

@fettuccinae fettuccinae requested a review from MonkeyDo September 5, 2025 17:55
@MonkeyDo
Copy link
Member

@MonkeyDo Done. Small question: why do we use POST to fetch data instead of GET in route loader? My understanding was that to use GET to read and POST to write .

Yes indeed you are right, and we know it looks wrong.
However at the moment we haven't found a good way to set up our single-page application for use with flask other than:

  1. hit a GET endpoint to render and send the HTML page template and JS bundles
  2. react gets activated on the page, front-end routing takes over
  3. hit a POST endpoint of the same name as the current route to fetch data for that component

And example of that here:

@donors_bp.get("/", defaults={"path": ""})
@donors_bp.get("/<path:path>/")
def donors(path):
return render_template("index.html")
@donors_bp.post("/")
def donors_post():

@MonkeyDo
Copy link
Member

I am hitting an error when I try to load the page, something to do with Oauth grant type.
Would appreciate some help figuring out what I have that's not correctly set up, or alternatively if you are hitting the same issue.

172.20.0.1 - - [30/Sep/2025 12:38:27] "POST /settings/notifications/ HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 1536, in __call__
    return self.wsgi_app(environ, start_response)
           ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 1514, in wsgi_app
    response = self.handle_exception(e)
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 1511, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 919, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 917, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python3.13/site-packages/flask_debugtoolbar/__init__.py", line 222, in dispatch_request
    return view_func(**view_args)
  File "/code/listenbrainz/listenbrainz/webserver/login/__init__.py", line 77, in decorated
    return f(*args, **kwargs)
  File "/code/listenbrainz/listenbrainz/webserver/views/settings.py", line 383, in get_digest_setting
    data = get_digest_preference(user["musicbrainz_row_id"])
  File "/code/listenbrainz/listenbrainz/domain/metabrainz_notifications.py", line 94, in get_digest_preference
    token = _fetch_token()
  File "/code/listenbrainz/listenbrainz/domain/metabrainz_notifications.py", line 144, in _fetch_token
    token = oauth.fetch_token(token_url=token_url, auth=HTTPBasicAuth(client_id, client_secret))
  File "/usr/local/lib/python3.13/site-packages/requests_oauthlib/oauth2_session.py", line 406, in fetch_token
    self._client.parse_request_body_response(r.text, scope=self.scope)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/clients/base.py", line 426, in parse_request_body_response
    self.token = parse_token_response(body, scope=scope)
                 ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/parameters.py", line 444, in parse_token_response
    validate_token_parameters(params)
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/parameters.py", line 451, in validate_token_parameters
    raise_from_error(params.get('error'), params)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/errors.py", line 398, in raise_from_error
    raise cls(**kwargs)
    
oauthlib.oauth2.rfc6749.errors.UnsupportedGrantTypeError: (unsupported_grant_type) 

@fettuccinae
Copy link
Contributor Author

fettuccinae commented Sep 30, 2025

I am hitting an error when I try to load the page, something to do with Oauth grant type. Would appreciate some help figuring out what I have that's not correctly set up, or alternatively if you are hitting the same issue.

172.20.0.1 - - [30/Sep/2025 12:38:27] "POST /settings/notifications/ HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 1536, in __call__
    return self.wsgi_app(environ, start_response)
           ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 1514, in wsgi_app
    response = self.handle_exception(e)
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 1511, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 919, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.13/site-packages/flask/app.py", line 917, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python3.13/site-packages/flask_debugtoolbar/__init__.py", line 222, in dispatch_request
    return view_func(**view_args)
  File "/code/listenbrainz/listenbrainz/webserver/login/__init__.py", line 77, in decorated
    return f(*args, **kwargs)
  File "/code/listenbrainz/listenbrainz/webserver/views/settings.py", line 383, in get_digest_setting
    data = get_digest_preference(user["musicbrainz_row_id"])
  File "/code/listenbrainz/listenbrainz/domain/metabrainz_notifications.py", line 94, in get_digest_preference
    token = _fetch_token()
  File "/code/listenbrainz/listenbrainz/domain/metabrainz_notifications.py", line 144, in _fetch_token
    token = oauth.fetch_token(token_url=token_url, auth=HTTPBasicAuth(client_id, client_secret))
  File "/usr/local/lib/python3.13/site-packages/requests_oauthlib/oauth2_session.py", line 406, in fetch_token
    self._client.parse_request_body_response(r.text, scope=self.scope)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/clients/base.py", line 426, in parse_request_body_response
    self.token = parse_token_response(body, scope=scope)
                 ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/parameters.py", line 444, in parse_token_response
    validate_token_parameters(params)
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/parameters.py", line 451, in validate_token_parameters
    raise_from_error(params.get('error'), params)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/oauthlib/oauth2/rfc6749/errors.py", line 398, in raise_from_error
    raise cls(**kwargs)
    
oauthlib.oauth2.rfc6749.errors.UnsupportedGrantTypeError: (unsupported_grant_type) 

The get_digest_preference function here, is trying to generate an access token from MeB.org but the metabrainz-notification branch havent been merged there yet and hence the error. Can you please clone the metabrainz-notification branch locally and try running both MeB and LB containers on the same docker network or you can just hard code the data returned by get_digest_preference if thats alright.

@fettuccinae
Copy link
Contributor Author

@MonkeyDo Done. Small question: why do we use POST to fetch data instead of GET in route loader? My understanding was that to use GET to read and POST to write .

Yes indeed you are right, and we know it looks wrong. However at the moment we haven't found a good way to set up our single-page application for use with flask other than:

1. hit a GET endpoint to render and send the HTML page template and JS bundles

2. react gets activated on the page, front-end routing takes over

3. hit a POST endpoint of the same name as the current route to fetch data for that component

And example of that here:

@donors_bp.get("/", defaults={"path": ""})
@donors_bp.get("/<path:path>/")
def donors(path):
return render_template("index.html")
@donors_bp.post("/")
def donors_post():

Makes sense!

@MonkeyDo
Copy link
Member

Makes sense. But could we just not make another GET endpoint with sole purpose of fetching

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.
Another option would be to use the same suffix for every route, say for example "-data": if you hit GET /user/$username/ to get the HTML page, the RouteLoader would hit GET /user/$username-data/
Another option yet, and probably the way to go, would be to use a custom request header to signify we want data for the route, not the HTML page.

@MonkeyDo
Copy link
Member

The get_digest_preference function here, is trying to generate an access token from MeB.org but the metabrainz-notification branch havent been merged there yet and hence the error. Can you please clone the metabrainz-notification branch locally and try running both MeB and LB containers on the same docker network or you can just hard code the data returned by get_digest_preference if thats alright.

Thanks, I hardcoded the return value, I'm only looking at checking the UI one last time

Copy link
Member

@MonkeyDo MonkeyDo left a 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 :)

@MonkeyDo MonkeyDo merged commit 2f065ce into metabrainz:metabrainz-notifications Sep 30, 2025
2 checks passed
@fettuccinae
Copy link
Contributor Author

Makes sense. But could we just not make another GET endpoint with sole purpose of fetching

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. Another option would be to use the same suffix for every route, say for example "-data": if you hit GET /user/$username/ to get the HTML page, the RouteLoader would hit GET /user/$username-data/ Another option yet, and probably the way to go, would be to use a custom request header to signify we want data for the route, not the HTML page.

Ohh, got it! Thanks for the detailed explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants