SHARE-533 Email templates & preview #3779

EifonUser · 2023-02-26T14:25:22Z

Your checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

Additional Description

TODO: Add additional information that is not in your commit-message here

Tests - additional information

TODO: add additional information about testruns here

codecov · 2023-02-26T14:56:27Z

Codecov Report

Merging #3779 (1744d5b) into develop (010b15c) will increase coverage by 19.03%.
The diff coverage is 1.66%.

@@              Coverage Diff               @@
##             develop    #3779       +/-   ##
==============================================
+ Coverage      30.85%   49.89%   +19.03%     
- Complexity      5713     5730       +17     
==============================================
  Files            654      654               
  Lines          20474    20530       +56     
==============================================
+ Hits            6318    10243     +3925     
+ Misses         14156    10287     -3869

Flag	Coverage Δ
behat	`47.73% <1.66%> (+19.96%)`	⬆️
phpunit	`8.68% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../Tools/SendMailToUser/SendMailToUserController.php	`0.00% <0.00%> (ø)`
...m/Testing/Behat/Context/CatrowebBrowserContext.php	`21.84% <0.00%> (+16.06%)`	⬆️
...Admin/Tools/SendMailToUser/SendMailToUserAdmin.php	`100.00% <100.00%> (ø)`

... and 232 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dmetzner

What I have noticed during testing

in the case of the preview - the user name should be optional

All in all the feature looks good :)

dmetzner · 2023-03-03T10:31:22Z

assets/js/custom/AdminMail.js

@@ -2,14 +2,15 @@

 // eslint-disable-next-line no-unused-vars
 function AdminMail () {
-  $('.btn').click(function () {
+  $('.btn-send').click(function () {


It would be great to refactor this code without jquery, since it is already touched in this MR - (should be do-able by chatgpt in a few seconds ;)

dmetzner · 2023-03-03T10:35:27Z

src/Admin/Tools/SendMailToUser/SendMailToUserController.php

+    $resetToken = $this->resetPasswordHelper->generateFakeResetToken();
+
+    $signature = 'https:://example.url';
+
+    $template = (string) $request->query->get('template');
+
+    $user = $this->user_manager->findUserByUsername((string) $request->query->get('username'));
+    if (!$user) {
+      return new Response('User does not exist');
+    }
+
+    $subject = (string) $request->query->get('subject');
+    if ('' === $subject && '2' === $template) {
+      return new Response('Empty subject!');
+    }
+
+    $titel = (string) $request->query->get('titel');
+
+    $messageText = (string) $request->query->get('message');
+    if ('' === $messageText && '2' === $template) {
+      return new Response('Empty message!');
+    }


would be nice to execute each snippet only if it is necessary - think of this previewAction as delegator

switch $template:
case A: load fake data for A - render
case B: load fake data for B - render

Feel free to create multiple classes or at least methods in a way that each class/method only has one job

dmetzner · 2023-03-03T10:37:22Z

templates/Admin/Tools/send_mail_to_user.html.twig

+              <option value="0">Confirmation Email</option>
+              <option value="1">Reset Email</option>
+              <option value="2">Simple Message</option>


using names instead of numbers makes the code more readable

Suggested change

<option value="0">Confirmation Email</option>

<option value="1">Reset Email</option>

<option value="2">Simple Message</option>

<option value="confirmation">Confirmation Email</option>

<option value="reset">Reset Email</option>

<option value="basic">Simple Message</option>

dmetzner · 2023-04-21T05:10:57Z

assets/js/custom/AdminMail.js

+    const subject = subjectInput.value
+    const titel = titelInput.value
+    const message = messageInput.value
+    const url = `send?username=${username}&subject=${subject}&titel=${titel}&message=${message}`


This will break on specific inputs. For example, let's consider you have the following example:

username = "user1"
subject = "Password Reset"
title = "...
....

Then you will get the following URL:

URL = "send?username=user1&subject=Password Reset&title=...

Since the parameters are not url-encoded, characters such as the space character will break your URL resulting into just fetching "send?username=user1&subject=Password"

window.open() encodes the url parameter by itself

If you want i could also use a json body to pass the parameters. I guess this would be more convenient

Is this still an issue? I didnt see any problem during testing, is it resolved @dmetzner ?

dmetzner · 2023-04-21T05:12:16Z

assets/js/custom/AdminMail.js

+        }
+      })
+      .then(data => {
+        if (data && data.length >= 2 && data.substring(0, 2) === 'OK') {


you should just check if the status code is 200, not check for the message content

The status code gets already checked above. If you dont like this code i can come up with a new structure that does the same.

dmetzner · 2023-04-21T05:12:38Z

src/Admin/Tools/SendMailToUser/SendMailToUserController.php

+        return $this->renderBasic($request);
+    }
+
+    return new Response('404');


The second param is the status code:

return new Response('Not Found', Response::HTTP_NOT_FOUND);

dmetzner · 2023-04-21T05:13:29Z

src/Admin/Tools/SendMailToUser/SendMailToUserController.php

+  {
+    $user = $this->user_manager->findUserByUsername((string) $request->query->get('username'));
+    if (!$user) {
+      return new Response('User does not exist');


dmetzner · 2023-04-21T05:13:37Z

src/Admin/Tools/SendMailToUser/SendMailToUserController.php

+
+    $subject = (string) $request->query->get('subject');
+    if ('' === $subject) {
+      return new Response('Empty subject!');


dmetzner · 2023-04-21T05:13:48Z

src/Admin/Tools/SendMailToUser/SendMailToUserController.php

+
+    $messageText = (string) $request->query->get('message');
+    if ('' === $messageText) {
+      return new Response('Empty message!');


dmetzner · 2023-04-21T05:14:35Z

assets/js/custom/AdminMail.js

+    const titel = titelInput.value
+    const message = messageInput.value
+    const template = templateSelect.value
+    const url = `preview?username=${username}&subject=${subject}&titel=${titel}&message=${message}&template=${template}`


same url encoding issue

hungryapeman · 2023-06-03T10:16:11Z

In the reset password preview, you can see the translation is not correct for "hour" its "Stunde", but not sure if that belongs in this PR:

hungryapeman · 2023-06-03T10:20:04Z

Would it make sense to disable certain fields in the reset and confirmation mail form like Content and Subject/Title, as they arent used in those to mails right?

EifonUser · 2023-06-03T13:24:07Z

This PR is more like a setup for SHARE-525 New E-Mail design.
If we would disable the unused preview fields, we would block the current "Send" option, which requires them.
For some reason, the string resetPassword.email.visitLink, is not translated in the crowdin_DE file on my branch. The translation for "hour" is present. On the current dev branch, the string is translated so it should not be an issue.
We could think of a solution, where there is either everything, or nothing translated. Anyways, this issue is not related to the preview, but to the current email template.

hungryapeman · 2023-06-03T16:17:12Z

This PR is more like a setup for SHARE-525 New E-Mail design. If we would disable the unused preview fields, we would block the current "Send" option, which requires them. For some reason, the string resetPassword.email.visitLink, is not translated in the crowdin_DE file on my branch. The translation for "hour" is present. On the current dev branch, the string is translated so it should not be an issue. We could think of a solution, where there is either everything, or nothing translated. Anyways, this issue is not related to the preview, but to the current email template.

Ah ok, then it looks fine for me! :)

SHARE-533 Email templates & preview

911e5ac

EifonUser added 3 commits February 27, 2023 21:20

Update SendMailToUserController.php

698c471

Update SendMailToUserController.php

d636243

Update SendMailToUserController.php

d2c8b18

dmetzner requested changes Mar 3, 2023

View reviewed changes

dmetzner assigned EifonUser Mar 3, 2023

dmetzner reviewed Mar 3, 2023

View reviewed changes

dmetzner marked this pull request as draft March 3, 2023 10:40

David Url and others added 5 commits March 16, 2023 04:46

SHARE-533 Email templates & preview

a6ca0a2

SHARE-533 Email templates & preview

ed8b491

Update mail_preview.feature

408bd3d

Merge branch 'Catrobat:develop' into SHARE-533

7b7aa02

SHARE-533 Email templates & preview

e6346af

EifonUser marked this pull request as ready for review April 19, 2023 19:00

dmetzner requested changes Apr 21, 2023

View reviewed changes

dmetzner marked this pull request as draft April 21, 2023 05:15

David Url added 2 commits April 21, 2023 17:02

SHARE-533 Email templates & preview

4d0412b

SHARE-533 Email templates & preview

1744d5b

EifonUser marked this pull request as ready for review April 28, 2023 12:57

dmetzner requested review from schaubes and hungryapeman June 2, 2023 13:55

hungryapeman approved these changes Jun 3, 2023

View reviewed changes

dmetzner self-requested a review June 9, 2023 07:28

dmetzner approved these changes Jun 9, 2023

View reviewed changes

dmetzner merged commit 7e360b2 into Catrobat:develop Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SHARE-533 Email templates & preview #3779

SHARE-533 Email templates & preview #3779

EifonUser commented Feb 26, 2023

codecov bot commented Feb 26, 2023 •

edited

Loading

dmetzner left a comment

dmetzner Mar 3, 2023

dmetzner Mar 3, 2023

dmetzner Mar 3, 2023

dmetzner Apr 21, 2023

EifonUser Apr 21, 2023

EifonUser Apr 21, 2023

hungryapeman Jun 3, 2023

dmetzner Apr 21, 2023

EifonUser Apr 21, 2023

dmetzner Apr 21, 2023

dmetzner Apr 21, 2023

dmetzner Apr 21, 2023

dmetzner Apr 21, 2023

dmetzner Apr 21, 2023

hungryapeman commented Jun 3, 2023

hungryapeman commented Jun 3, 2023

EifonUser commented Jun 3, 2023

hungryapeman commented Jun 3, 2023

SHARE-533 Email templates & preview #3779

SHARE-533 Email templates & preview #3779

Conversation

EifonUser commented Feb 26, 2023

Your checklist for this pull request

Additional Description

Tests - additional information

codecov bot commented Feb 26, 2023 • edited Loading

Codecov Report

dmetzner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hungryapeman commented Jun 3, 2023

hungryapeman commented Jun 3, 2023

EifonUser commented Jun 3, 2023

hungryapeman commented Jun 3, 2023

codecov bot commented Feb 26, 2023 •

edited

Loading