Send mails in chunks #1509
Send mails in chunks #1509
Conversation
UrsMetz
force-pushed
the
send-mails-in-chunks
branch
from
9b41218
to
ae0a279
Compare
| logger.error(err); | ||
| }); | ||
|
|
||
| return mailtransport.statusmessageForSuccess(); |
There was a problem hiding this comment.
DAs kommt zurück, bevor die Statusnachricht rausgeht.
There was a problem hiding this comment.
War auch mein Plan: da ja der Versand der ganzen Mails zu lange dauert, dachte ich, am besten das ganze im Hintergrund machen und schonmal vom express-Handler ein Ok rausgeben, damit man sicher kein Timeout bekommt. Ist das problematisch?
There was a problem hiding this comment.
Oder anders gefragt. Welche der Umsetzungsoptionen soll ich machen:
- wenn man mehr als die max. Chunk-Size (deduplizierte) BCCs hat, dann wird an die UI gleich ein: ist unterwegs zurückgegeben, auch wenn die Mails noch verschickt werden (also den Ansatz den ich hier gemacht habe). Wenn das alles durch ist, dann wird eine Mail an den Absender verschickt, mit dem Versende-Report.
- oder man verschickt doch alle Mails (aber eben in mehrere Chunks aufgeteilt) und erst dann gibt man ein Ergebnis. Da man dann aber auch weiß ob alles geklappt hat, braucht man dann überhaupt den Versende-Report per Mail?
| return resultMessage; | ||
| } | ||
|
|
||
| function sendMailInChunks(maxMailSendingChunkSize, allMembers, message, type, sender) { |
There was a problem hiding this comment.
Kannst Du das in async/await umwandeln und das statushandling geradeziehen? - Fehler oder Success zurückgeben
There was a problem hiding this comment.
Die Umsetzung davon hängt glaube ich auch an er Frage, welche der beiden Optionen (s. oben) umgesetzt wird.
Wenn man die Mails im Hintergrund versendet, dann ist es meiner Meinung nach mit Promise.then einfacher bzw. dann kann die sendInChunks zwar await verwenden, aber der Aufrufer von sendInChunks darf nicht await verwenden.
Bei der zweiten Option ist await auf jeden Fall die schönere Variante.
| @@ -106,7 +143,8 @@ module.exports = { | |||
| } | |||
| try { | |||
| groups.forEach(groupsAndMembersService.addMembersToGroup); | |||
| message.setBccToGroupMemberAddresses(groups); | |||
| const allMembers = R.flatten(misc.compact(groups).map((group) => group.members)); | |||
|
|
|||
There was a problem hiding this comment.
Ist das nicht einfach ein "groups.flatMap(group >= group.members)" plus duplikate bereinigen? - bzw. da sind doch duplikate drin!
There was a problem hiding this comment.
Kann gut sein, muss ich mir anschauen :-).
| @@ -140,11 +185,10 @@ module.exports = { | |||
| } | |||
| }, | |||
|
|
|||
| sendMailToAllMembers: async function sendMailToAllMembers(message) { | |||
| sendMailToAllMembers: async function sendMailToAllMembers(message, sender) { | |||
There was a problem hiding this comment.
nochmal die frage nach dem sender, der ist doch in der message schon drin?
There was a problem hiding this comment.
Nein, nur der Name und die Mailadresse, daraus wieder einen Member zu bauen, fand ich etwas doof bzw. war mir auch nicht sicher, ob das dann immer so klappt.
Ich wollte auch die Message nicht um eine weiteres Feld erweitern (eben den sender), aber wenn das ok ist, kann ich das auch machen.
| const sentMail = singleSentEmail(); | ||
| expect(sentMail.bcc).to.contain("memberA"); | ||
| expect(sentMail.html).to.contain("mark down"); | ||
| expect(sentMail.icalEvent).to.not.contain("BEGIN:VCALENDAR"); | ||
| expect(sentMail.icalEvent).to.not.contain("URL:http://localhost:17125/activities/urlOfTheActivity"); | ||
| expect(statusmessage.contents().type).to.equal("alert-success"); | ||
| }); | ||
|
|
||
| describe("sending in chunks", () => { |
There was a problem hiding this comment.
please add test where same member is twice, as first and last address
| expect(allSent[2].html).to.contain("erfolgreich"); | ||
| }); | ||
|
|
||
| it("returns success regardless of mail sending result", async () => { |
There was a problem hiding this comment.
Das find ich nicht gut. Status in erfolgsmail soll status von meldung sein
There was a problem hiding this comment.
Bei meinen manuellen Tests hat aber die Ersetzung der i18n-Keys nicht stattgefunden (und die Frage ist ja auch welches locale man dann verwenden soll bzw. wie man das durschleift?).
|
Die Tests habe ich mir noch nicht im Detail angesehen |
|
|
||
| Promise.all( | ||
| membersInChunks.map((bccs) => { | ||
| message.setBccToMemberAddresses(bccs); |
There was a problem hiding this comment.
Hier werden die Duplikate beseitigt, allerdings in der neuen Version kann derselbe member in mehreren chunks sein!
There was a problem hiding this comment.
Stimmt, nicht dran gedacht. Ist zwar etwas unschön, aber dann muss ich das Duplikatefiltern eben im Service machen...
|
Danke fürs Feedback @leider, schaue ich mir die nächsten Tage genau an, heute schaffe ich das nicht mehr. |
Für die Mails an Gruppen (Einladungen als auch Mails an alle in der Gruppe) und die Superuser-Funktion an alle Members zu senden, erlaubt die Änderung in diesem PR, dass man die Mails in einer konfigurierbaren Chunk-Aufteilung versendet.
Damit das Versenden noch geht, muss zwingend eine maximale Chunk-Größe konfigurieren, daher habe ich auch die
mailsender-config.jsoninconfig-examplesangepasst.Hoffentlich fixes #1456