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.

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

MBS-13815: Enable sending contact emails through new service #3390

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

JadedBlueEyes
Copy link
Contributor

@JadedBlueEyes JadedBlueEyes commented Oct 21, 2024

Problem

This is the start of enabling mb-mail-service incrementally, as a broken-down version of #3363.
This adds the configuration option MAIL_SERVICE_BASE_URL in DBdefs, and replaces send_message_to_editor (aka /user/<id>/contact) with the new template.

Testing

This has been manually tested.

Documenting

Further action

  1. Ensure beta.musicbrainz.org is configured correctly before deploying
  2. Write replacement tests for the new code
  3. Once this has seen some success, enable more new templates

Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

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

Thanks for splitting this, I hope we can put it on test/beta soon!

lib/MusicBrainz/Server/Email.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Email.pm Show resolved Hide resolved
lib/MusicBrainz/Server/Email.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Email.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Email.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Email.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Email.pm Outdated Show resolved Hide resolved
@JadedBlueEyes
Copy link
Contributor Author

Heya, sorry for missing your review @mwiencek! I've implemented the suggestions, although the comment about languages made me realise that this particular one is being sent to two different users with potentially two different languages - I've added comments to that effect, but when we get around to adding the language preference we'll need to decide whether to send the email in different languages to the two participants or to use the initiator or recipient's language. A similar thing applies to the report emails.

@mwiencek
Copy link
Member

@JadedBlueEyes Thanks so much for the update and sorry for the delay. I just pushed some minor fixes/improvements:

  • Rebased on master
  • Rewrote the old tests (they now just check what JSON we're sending to the mb-mail-service -- tests for the actual email content probably belong in that repo now)
  • Fixed a couple small bugs I found (one was due to accessing $res->status instead of $res->code, the other was af68895)
  • Made a couple small style changes (removed unnecessary key quotes, sorted imports)

I think it's ready to put on beta, so I'll try to do that tomorrow.

I've implemented the suggestions, although the comment about languages made me realise that this particular one is being sent to two different users with potentially two different languages - I've added comments to that effect, but when we get around to adding the language preference we'll need to decide whether to send the email in different languages to the two participants or to use the initiator or recipient's language. A similar thing applies to the report emails.

Good point. I think we should change the language depending on the recipient in both cases, but we can revisit that once the preference is added.

@reosarevok
Copy link
Member

It should 100% be either each language, or English for now until that's doable if it cannot be done yet. Please make sure you don't end up sending a French email to a non-French speaker or whatnot just because the other user has the site in French, that sounds awful.

JadedBlueEyes and others added 3 commits November 12, 2024 10:11
This page was shown even if the email didn't send: the redirect was
unconditional, and the "sent" parameter was always included, which made the
`exists` check for that parameter completely useless.

Now it reloads the form (preserving what was entered).
@reosarevok
Copy link
Member

Fixed the Perl::Critic errors here, hopefully without breaking anything else.

@mwiencek mwiencek merged commit a468170 into metabrainz:master Nov 13, 2024
1 of 2 checks passed
@mwiencek mwiencek changed the title MBS-13719: Enable sending contact emails through new service MBS-13815: Enable sending contact emails through new service Nov 13, 2024
@mwiencek
Copy link
Member

I deployed this to beta and it seems to be working fine (sent a message to Jade as a test). One issue I overlooked, though, is that the message subject is ignored and not sent to the recipient at all. This is a problem with the mb-mail-service template and not the MBS changes here.

@reosarevok
Copy link
Member

We should aim to fix that before this goes out of beta - or revert this. Are you already looking into it?

@JadedBlueEyes
Copy link
Contributor Author

sent a message to Jade as a test

Received!

One issue I overlooked, though, is that the message subject is ignored and not sent to the recipient at all. This is a problem with the mb-mail-service template and not the MBS changes here.

I've just done this in metabrainz/mb-mail-service@cb44ed0 / 0.3.7, although it might need a bit of design polish :)

image

I've also realised that for the image, we're using https://static.metabrainz.org/MB/header-logo-1f7dc2a.svg, which isn't a permalink, and also is an SVG, which can't display in some clients. Can we get a permalink for a high-quality PNG of the header logo?

@mwiencek
Copy link
Member

Thank you @JadedBlueEyes!

I'm now wondering if there's a point to having a subject anymore if it's only to display some bold text above the message, and not be used in the actual email subject.

I checked how Discogs handles this, and the string they use for the email subject is "Discogs message from {user}: {subject}". What do you think about preserving the subject in the email in a similar way?

Would like some feedback from @Aerozol too. :)

@mwiencek
Copy link
Member

I've also realised that for the image, we're using https://static.metabrainz.org/MB/header-logo-1f7dc2a.svg, which isn't a permalink, and also is an SVG, which can't display in some clients. Can we get a permalink for a high-quality PNG of the header logo?

Do you have a suggestion for what size the png should be? (I assume the current CSS will scale it appropriately?)

@Aerozol
Copy link
Contributor

Aerozol commented Nov 13, 2024

Hmm I guess it doesn't hurt to allow the sender to "title" their paragraph, for instance if they are sending multiple messages on different topics to the same user? If "subject" is going to be expected to be in the actual email subject maybe we can change that field to "Title" in the MB end, to avoid confusion.

Layout wise (assuming we keep the subject/title) I propose a couple of possible changes:

  1. Remove the bold "Tester" from inside the speech bubble entirely. Instead, bold "Tester" in the message above ("MusicBrainz user 'Tester' has sent...)
  2. Keep Tester and the subject/title on the same line ("Tester: Test Subject/Title"

P.S. The PNG already in the Design System might be about the right size: https://github.com/metabrainz/design-system/tree/master/brand/logos/MusicBrainz/PNG

@mwiencek
Copy link
Member

Hmm I guess it doesn't hurt to allow the sender to "title" their paragraph, for instance if they are sending multiple messages on different topics to the same user? If "subject" is going to be expected to be in the actual email subject maybe we can change that field to "Title" in the MB end, to avoid confusion.

In production we currently include the subject in the email subject, which seems useful, but in beta it's now You've received a new message from {user}. I don't mind also showing the subject in the email body, but is there a reason to not use something like You've received a new message from {user}: {subject} for the email subject?

@JadedBlueEyes
Copy link
Contributor Author

  • Having the email subject as You've received a new message from {user}: {subject} seems good to me (this will need to be updated in translations too)
  • I would prefer to bold the username in the body and remove it from the speech bubble; however, we currently don't have a way of including HTML/formatting in translation strings. For now, we can use the second suggestion ({user}: {subject} in the bubble)

If that sounds good, I'll implement those changes tomorrow.

This looks good for me, we just need to host it somewhere now :)

@Aerozol
Copy link
Contributor

Aerozol commented Nov 13, 2024

Oh cool, I interpreted this discussion as us not being able to put it in the actual email subject. Now that you mention it, it has sometimes been confusing to get MB emails that are just a random subject...

I think my preference for the subject would be:
MusicBrainz message from {user}: {subject}
Which would tell me everything I need to know, when browsing my inbox.

@JadedBlueEyes make sure to use the MusicBrainz_logo_mini.png, not the chonky one.

@mwiencek
Copy link
Member

@JadedBlueEyes make sure to use the MusicBrainz_logo_mini.png, not the chonky one.

Uploaded to https://static.metabrainz.org/logos/MusicBrainz_logo_mini.png.

@JadedBlueEyes
Copy link
Contributor Author

I've made the changes now, they should be released in image v0.3.8!

image

@mwiencek
Copy link
Member

Deployed that, thanks!

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.

4 participants