feat: add request info to all emails #431

jozefrebjak · 2022-09-10T10:44:40Z

It is common practice to include in email verification emails some user request info like IP Address, User Agent Device & Date when a user is asked for some code. This PR adds this information.

I also improved default email templates to improve readability of the code.

@lonnieezell @kenjis @MGatner I don't know if we want to provide more complex Email templates, or if basic HTML is fine for the start of using this library and leave it completely to the developer.

Language files are not completed, some translated with Google Translator.

Please help with translations.

Here is what it looks like now in the Email client.

kenjis · 2022-09-10T12:21:04Z

some translated with Google Translator.

Can we use translations by Google Translator in this project?
Do we have copyright of the translation?
And we can certify the following?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/DCO.md

datamweb · 2022-09-11T04:48:14Z

Hello @jozefrebjak ,
Thank you for sending PR.

I agree to the following terms with this PR:

IP Address, User Agent Device & Date

Please add Time.

I also improved default email templates to improve readability of the code.
I don't know if we want to provide more complex Email templates, or if basic HTML is fine for the start of using this library and leave it completely to the developer.

I am against adding and changing the default templates, the reason is that in a real project each site has its own theme and the email templates are the same as the site theme, so I believe that in the real project the email templates are also The face is designed exclusively and we do not need to add details in the shield.

Please help with translations.

In the final stages of this PR, we can get help from our friends for translation. Therefore, we must first see what the members think.

Refactor
In my opinion, this PR should be implemented by View Decorators. I believe it makes us write less code and provides better flexibility. I want to hear from other members here?

datamweb · 2022-09-11T04:56:45Z

src/Language/en/Auth.php

@@ -64,6 +64,12 @@
    'userDoesNotExist'          => 'Password was not changed. User does not exist',
    'resetTokenExpired'         => 'Sorry. Your reset token has expired.',

+    // Email Globals
+    'emailInfo'      => 'Some information about the person who asked for the code:',


Please write in such a way that it covers the magic link as well.

@datamweb I'd rather add a new line for the magic link.

I don't see the need to add a new line on this.
You can use the phrase "Applicant information:" or something similar. Because in magic-link we do not send code, a link is sent.
However, if you think adding new line would be useful for the Shield, I have no problem with it.

MGatner · 2022-09-11T10:47:25Z

Can we use translations by Google Translator

An intriguing question! I did some digging, and the answer is murky but "yes". Here is an excerpt from WikiMedia's legal team who had to answer this for their services:

Assuming that such a mechanical translation is a protected derivative work ... the resulting copyright owner would be the user of the software, not the software manufacturer. ... Since the Supreme Court defined “author” as “he to whom anything owes its origin,”[7] it follows that person who initiated the translation would be the author.

So two interpretations:

The translation is too shallow a change to count as a derivative work so copyright belongs solely to the originating author
The translation is derivative but is done via automation so the copyright is the author's but the license for the derivative version belongs to the user of Google Translate

In both cases since @jozefrebjak is the originating author and the one using the translation software he is the sole owner of the content and may submit it based on our license and agreement.

kenjis · 2022-09-11T11:43:25Z

@MGatner Very interesting opinion. Can you give the URL of the WikiMedia's legal team answer?

MGatner · 2022-09-11T11:45:48Z

Here ya go: https://meta.m.wikimedia.org/wiki/Wikilegal/Copyright_for_Google_Translations

Specifically "Copyright in Translations"

src/Language/ja/Auth.php

src/Views/Email/email_2fa_email.php

kenjis · 2022-09-14T00:53:46Z

Please fix the failed test: 1) Tests\Controllers\ActionsTest::testEmailActivateShow

kenjis · 2022-09-14T01:00:16Z

Untranslated language files cause test failures.
What if you put some marks as follows for the ones that cannot be translated?

--- a/src/Language/fa/Auth.php
+++ b/src/Language/fa/Auth.php
@@ -74,10 +74,10 @@
     'resetTokenExpired'         => 'متاسفانه، توکن بازنشانی شما منقضی شده است.',
 
     // Email Globals
-    'emailInfo'      => 'Some information about the person who asked for the code:',
-    'emailIpAddress' => 'IP Address:',
-    'emailDevice'    => 'Device:',
-    'emailDate'      => 'Date:',
+    'emailInfo'      => '(To be translated) Some information about the person who asked for the code:',
+    'emailIpAddress' => '(To be translated) IP Address:',
+    'emailDevice'    => '(To be translated) Device:',
+    'emailDate'      => '(To be translated) Date:',
 
     // 2FA
     'email2FATitle'       => 'احراز هویت دو عاملی',

jozefrebjak · 2022-09-15T14:52:56Z

Untranslated language files cause test failures. What if you put some marks as follows for the ones that cannot be translated?

--- a/src/Language/fa/Auth.php
+++ b/src/Language/fa/Auth.php
@@ -74,10 +74,10 @@
     'resetTokenExpired'         => 'متاسفانه، توکن بازنشانی شما منقضی شده است.',
 
     // Email Globals
-    'emailInfo'      => 'Some information about the person who asked for the code:',
-    'emailIpAddress' => 'IP Address:',
-    'emailDevice'    => 'Device:',
-    'emailDate'      => 'Date:',
+    'emailInfo'      => '(To be translated) Some information about the person who asked for the code:',
+    'emailIpAddress' => '(To be translated) IP Address:',
+    'emailDevice'    => '(To be translated) Device:',
+    'emailDate'      => '(To be translated) Date:',
 
     // 2FA
     'email2FATitle'       => 'احراز هویت دو عاملی',

Good idea how to handle missing languages.

jozefrebjak · 2022-09-15T15:01:40Z

Hello @jozefrebjak , Thank you for sending PR.

I agree to the following terms with this PR:

IP Address, User Agent Device & Date

Please add Time.

I also improved default email templates to improve readability of the code.
I don't know if we want to provide more complex Email templates, or if basic HTML is fine for the start of using this library and leave it completely to the developer.

I am against adding and changing the default templates, the reason is that in a real project each site has its own theme and the email templates are the same as the site theme, so I believe that in the real project the email templates are also The face is designed exclusively and we do not need to add details in the shield.

Please help with translations.

In the final stages of this PR, we can get help from our friends for translation. Therefore, we must first see what the members think.

Refactor In my opinion, this PR should be implemented by View Decorators. I believe it makes us write less code and provides better flexibility. I want to hear from other members here?

For me is date enough, but I can change this to return also time.

Time::now()->toLocalizedString('MMM d, yyyy');

I'm fine with html email like they are now. I have added for now only a few basic HTML tags, which I believe we should support in default email templates.

I'd like to learn how to use View Decorators to provide some kind of templating system, but I don't have experience with that yet.

datamweb · 2022-09-16T02:59:59Z

For me is date enough, but I can change this to return also time.

Please add, maybe this is better. toDateTimeString

I'm fine with html email ...

Okay. Because you have made some changes, the test has an error, please correct the following to fix it.

shield/tests/Controllers/ActionsTest.php

Lines 233 to 241 in 5c1a9af

    
           // Should have sent an email with the link.... 
        
           $this->assertStringContainsString( 
        
               'Please use the code below to activate your account and start using the site', 
        
               service('email')->archive['body'] 
        
           ); 
        
           $this->assertMatchesRegularExpression( 
        
               '!<p>[0-9]{6}</p>!', 
        
               service('email')->archive['body'] 
        
           );

more info see : https://github.com/codeigniter4/shield/actions/runs/3061616247/jobs/4941607181

I'd like to learn how to use View Decorators ...

In this regard, since other members did not comment, there is no need to change. But if you want to learn, watch this video.

src/Language/fr/Auth.php

ddevsr

    'emailInfo'      => 'Beberapa informasi tentang seseorang yang meminta kode:',
    'emailIpAddress' => 'Alamat IP:',
    'emailDevice'    => 'Perangkat:',
    'emailDate'      => 'Tanggal:',

datamweb · 2022-09-16T09:28:27Z

@ddevsr thank you for your help.
You can register a change request in the review. please see here:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request

ddevsr · 2022-09-16T09:58:38Z

@ddevsr thank you for your help. You can register a change request in the review. please see here:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request

@datamweb Thankyou for information how to request changes

src/Language/id/Auth.php

datamweb · 2022-10-05T18:31:25Z

Hello @jozefrebjak , can you continue this PR?

datamweb · 2022-10-05T20:47:06Z

Some of your commits are unsigned, please check GPG-Signing Old Commits, also note other comments.

jozefrebjak · 2022-10-05T21:16:06Z

Some of your commits are unsigned, please check GPG-Signing Old Commits, also note other comments.

@datamweb I signed the latest commit, but the older ones were not signed. The rebase method is completely new to me, so if you provide me with how to sign them, I will.

kenjis · 2022-10-05T21:31:06Z

I recommend you set git sign automatically.

As a faster alternative, you can still securely sign commits without the -S option in git commit by setting git config --global commit.gpgsign true and git config --global user.signingkey FA4CD0840816FD28 to all local repositories. Without the --global option, the change is applied to one local repository only.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

Then, just to run git rebase signs all commits.

$ git fetch upstream
$ git rebase upstream/develop

Co-authored-by: kenjis <kenji.uui@gmail.com>

kenjis · 2022-10-06T11:41:41Z

I got it.

The last merge commit broke the branch.

2b86f82 - (HEAD -> jozefrebjak/develop) Merge branch 'develop' of github.com:jozefrebjak/shield into develop [2022-10-06 12:09:13 +0200]

All you need to do is:

reset the last commit
```
$ git reset HEAD^
```
force push

Resetting the last commit makes the git log graph straight.

* 8e958ae - (HEAD -> jozefrebjak/develop) Fix testEmailActivateShow [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
* 60f14a5 - Update src/Language/fr/Auth.php [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
* 8b4af59 - Update src/Language/id/Auth.php [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
* ee9ec4f - fix fr translate [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
* 0526ada - tests fix [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
* dd635e3 - Update src/Views/Email/email_2fa_email.php [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
* d71564c - Update src/Language/ja/Auth.php [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
* f3baef9 - feat: add request info to emails [2022-10-06 12:08:48 +0200] <Jozef Rebjak>
*   f4cdfb6 - (upstream/develop, test, develop) Merge pull request #464 from datamweb/fix-property-last_used_at [2022-10-05 13:41:44 +0330] <Pooya Parsa Dadashi>

src/Language/fa/Auth.php

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

datamweb · 2022-10-28T19:11:02Z

hello @jozefrebjak,
Please address the mentioned items, including Time and magic link.
What do you think about them?

jozefrebjak · 2022-10-29T07:42:24Z

hello @jozefrebjak,
Please address the mentioned items, including Time and magic link.
What do you think about them?

@datamweb This PR was about verification emails, I would like to make another PR about the magic link email.

datamweb

Sorry for the delay, Google translator is limited here.
The unit test doesn't fully cover it, however I don't mind.

Also note that we are in the process of releasing a new version, if possible send out the magic link PR as soon as possible. (Honestly, I believe that should have been done in this PR as well.)
Thank you.

jozefrebjak · 2022-10-30T19:08:17Z

Sorry for the delay, Google translator is limited here. The unit test doesn't fully cover it, however I don't mind.

Also note that we are in the process of releasing a new version, if possible send out the magic link PR as soon as possible. (Honestly, I believe that should have been done in this PR as well.) Thank you.

@datamweb Ok, please leave it open, I'll add support for magic link as well.

src/Views/Email/email_2fa_email.php

src/Views/Email/email_activate_email.php

jozefrebjak · 2022-10-31T10:54:19Z

@datamweb, @kenjis

Here is the final emails:

Register

2FA

Magic Link

datamweb

Thanks for the changes. good work.

src/Language/fa/Auth.php

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

kenjis · 2022-11-06T23:25:33Z

All checks have passed. I'm merging.

datamweb added the enhancement New feature or request label Sep 10, 2022

datamweb reviewed Sep 11, 2022

View reviewed changes

kenjis reviewed Sep 11, 2022

View reviewed changes

src/Language/ja/Auth.php Outdated Show resolved Hide resolved

kenjis reviewed Sep 11, 2022

View reviewed changes

src/Views/Email/email_2fa_email.php Outdated Show resolved Hide resolved

datamweb requested changes Sep 16, 2022

View reviewed changes

src/Language/fr/Auth.php Outdated Show resolved Hide resolved

ddevsr suggested changes Sep 16, 2022

View reviewed changes

src/Language/id/Auth.php Outdated Show resolved Hide resolved

datamweb added the GPG-Signing needed Pull requests that need GPG-Signing label Sep 17, 2022

datamweb added the waiting for info Issues or pull requests that need further clarification from the author label Oct 5, 2022

jozefrebjak force-pushed the develop branch from eacbfef to 9856618 Compare October 5, 2022 21:05

jozefrebjak and others added 4 commits October 6, 2022 12:08

feat: add request info to emails

f3baef9

Update src/Language/ja/Auth.php

d71564c

Co-authored-by: kenjis <kenji.uui@gmail.com>

Update src/Views/Email/email_2fa_email.php

dd635e3

Co-authored-by: kenjis <kenji.uui@gmail.com>

tests fix

0526ada

jozefrebjak force-pushed the develop branch from 2b86f82 to 8e958ae Compare October 6, 2022 18:36

datamweb removed the GPG-Signing needed Pull requests that need GPG-Signing label Oct 7, 2022

datamweb requested changes Oct 7, 2022

View reviewed changes

src/Language/fa/Auth.php Outdated Show resolved Hide resolved

src/Language/fa/Auth.php Outdated Show resolved Hide resolved

src/Language/fa/Auth.php Outdated Show resolved Hide resolved

src/Language/fa/Auth.php Outdated Show resolved Hide resolved

jozefrebjak and others added 4 commits October 8, 2022 02:33

Update src/Language/fa/Auth.php

f36ed55

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

Update src/Language/fa/Auth.php

e2afe23

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

Update src/Language/fa/Auth.php

45c99d8

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

Update src/Language/fa/Auth.php

a0440cb

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

jozefrebjak requested a review from datamweb October 28, 2022 18:59

jozefrebjak added 2 commits October 29, 2022 09:06

Merge branch 'codeigniter4:develop' into develop

eb54799

refactor: use toDateTimeString

76c34be

datamweb approved these changes Oct 30, 2022

View reviewed changes

datamweb removed the waiting for info Issues or pull requests that need further clarification from the author label Oct 30, 2022

datamweb requested a review from kenjis October 30, 2022 18:51

kenjis reviewed Oct 30, 2022

View reviewed changes

src/Views/Email/email_2fa_email.php Outdated Show resolved Hide resolved

kenjis reviewed Oct 30, 2022

View reviewed changes

src/Views/Email/email_activate_email.php Outdated Show resolved Hide resolved

magic-link

661516f

kenjis approved these changes Oct 31, 2022

View reviewed changes

datamweb requested changes Nov 1, 2022

View reviewed changes

src/Language/fa/Auth.php Outdated Show resolved Hide resolved

datamweb changed the title ~~feat: add request info to verification emails~~ feat: add request info to all emails Nov 1, 2022

Update src/Language/fa/Auth.php

2dd99db

Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>

datamweb approved these changes Nov 2, 2022

View reviewed changes

datamweb requested a review from MGatner November 2, 2022 08:04

kenjis merged commit dd18be0 into codeigniter4:develop Nov 6, 2022

feat: add request info to all emails #431

feat: add request info to all emails #431

Uh oh!

Conversation

jozefrebjak commented Sep 10, 2022 • edited by datamweb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenjis commented Sep 10, 2022

Uh oh!

datamweb commented Sep 11, 2022

Uh oh!

datamweb Sep 11, 2022

Choose a reason for hiding this comment

Uh oh!

jozefrebjak Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

datamweb Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

MGatner commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenjis commented Sep 11, 2022

Uh oh!

MGatner commented Sep 11, 2022

Uh oh!

Uh oh!

Uh oh!

kenjis commented Sep 14, 2022

Uh oh!

kenjis commented Sep 14, 2022

Uh oh!

jozefrebjak commented Sep 15, 2022

Uh oh!

jozefrebjak commented Sep 15, 2022

Uh oh!

datamweb commented Sep 16, 2022

Uh oh!

Uh oh!

ddevsr left a comment

Choose a reason for hiding this comment

Uh oh!

datamweb commented Sep 16, 2022

Uh oh!

ddevsr commented Sep 16, 2022

Uh oh!

Uh oh!

datamweb commented Oct 5, 2022

Uh oh!

datamweb commented Oct 5, 2022

Uh oh!

jozefrebjak commented Oct 5, 2022

Uh oh!

kenjis commented Oct 5, 2022 • edited by datamweb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenjis commented Oct 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

datamweb commented Oct 28, 2022

Uh oh!

jozefrebjak commented Oct 29, 2022

Uh oh!

datamweb left a comment

Choose a reason for hiding this comment

Uh oh!

jozefrebjak commented Oct 30, 2022

Uh oh!

Uh oh!

Uh oh!

jozefrebjak commented Oct 31, 2022

Uh oh!

datamweb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jozefrebjak commented Sep 10, 2022 •

edited by datamweb

Loading

MGatner commented Sep 11, 2022 •

edited

Loading

kenjis commented Oct 5, 2022 •

edited by datamweb

Loading