fix(mailer): get notification_mail when sending email #496
Conversation
lib/massmailer.js
Outdated
| @@ -46,7 +47,7 @@ exports.sendAll = async (req, res) => { | |||
| // TODO: Think, maybe use Pug or EJS for that? | |||
| // TODO: Think what else will we need? Probably remove after Agora Bucuresti if there | |||
| // won't be something required. | |||
| const email = application.email; | |||
| const notificationEmail = await core.getMember(req, application.user_id).notification_email; | |||
There was a problem hiding this comment.
this one will do like 800 requests to the core each time someone would want to use a massmailer, what I'd do instead is storing the notification email in the application, the same way and for the same reason body name and email is stored here, then you can send email without even querying core
There was a problem hiding this comment.
Yeah, but that will not update the notification_email so you might get the wrong one. You normally apply to events a few months in advance so the data is not very recent
There was a problem hiding this comment.
what do you think of doing it all with 1 query then?
There was a problem hiding this comment.
That would be awesome, but I wasn't quite sure how to do that. But I haven't given it too much thought yet so with some more time I probably could figure that out.
And figuring out why application creation fails 😅
There was a problem hiding this comment.
just use /members endpoint of core, it'll return them all, then you can just find the correct user in results. one issue here is that you have to have the global:view:member permission, otherwise the call to core will fail
There was a problem hiding this comment.
Would that actually be better? Then we would get a much bigger query back and I am unsure if that will be any quicker in the end. Having the global:view:member permission might not be needed if we use the superadmin information from config.
There was a problem hiding this comment.
depends on how much emails needs to be send, but in general, 1 big query is better than 500 smaller ones
There was a problem hiding this comment.
but this was also the reason why I added email to application, to reduce it to 0 and to not bother with permissions
There was a problem hiding this comment.
Yeah, adding the emails to applications is indeed a nice solution but does not work for fields that can change somewhere else unfortunately
There was a problem hiding this comment.
I've removed this out of this PR and made a TODO for it. I do want to merge the other changes since those features are broken currently.
Codecov Report
@@ Coverage Diff @@
## master #496 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 38 38
Lines 1877 1877
Branches 342 342
=========================================
Hits 1877 1877
Continue to review full report at Codecov.
|
Save normal email in application for exporting or whatever. Will be looked into later
WikiRik
force-pushed
the
notification_email
branch
from
b79e85b
to
d38790e
Compare
lib/applications.js
Outdated
| @@ -624,9 +624,11 @@ exports.postApplication = async (req, res) => { | |||
|
|
|||
| // We don't need to recalculate the votes amount, as the pax type is not set here. | |||
|
|
|||
| const notificationEmail = await core.getMember(req, newApplication.user_id).notification_email; | |||
There was a problem hiding this comment.
if it's the same user as the one who posts the application, you can just as well use req.user.notification_email, no need for extra request here
|
🎉 This PR is included in version 1.6.6 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Save normal email in application for exporting or whatever. Will be looked into later
Related to AEGEE/core#238