@peterhashair opened this Pull Request on November 19th 2021 Contributor

Description:

Just a note, I think this PR will be put on pause for now, currently is on the transaction from angular to VUE. Once the Vue implementation is finished, will convert it to VUE and fix the remaining task.

Add Two table column

  • invite_status sent or null
  • invite_at datetime

Using currently token from user_auth_token table. Resend invite will send a link to the user, clicking the link will mark the user active and land on the password setting page.

Email template:

image

Missing:

  • UI Test
  • Integration Test

Review

@tsteur commented on November 21st 2021 Member

@peterhashair I saw your questions in the email but they are removed in the comment here. I'll comment quick anyway

To addColumn to exiting database table. do I use `4.7.0-b1?

Likely yes, it depends when we merge it and whether by then maybe already a b1 has been released. Likely it will be -b1 though.

Introduce user status, currently depending on the invite_token field, just checking, do we want to introduce this field as an individual field not relied on invite_token. So it accepts pending, suspended, disabled, etc.

Not 100% sure. It really depends whether it's needed or not. It might not be needed as I think we delete a user if we withdraw the invite etc. We probably won't have a suspended. I'd say it's probably not needed, but it's hard to say without being too much into it.

Resend invites, after people send invites do we need a status said sent, and the next send can only be done after 15mins? Otherwise, it maybe adds a mail server to span?

Yes that be good 👍

@peterhashair commented on November 22nd 2021 Contributor

@tsteur sorry to bother you, I am trying to implement integration tests, but it seems like the database missing the new field I happened to, do I need to add a install() function inside of UserManager other than 4.7.0-b1.php

@tsteur commented on November 22nd 2021 Member

@peterhashair you will also need to adjust the schema in https://github.com/matomo-org/matomo/blob/4.6.0-rc2/core/Db/Schema/Mysql.php#L47 which is used for new installs

@peterhashair commented on November 22nd 2021 Contributor

@tsteur it seems like the tests still don't apply that database extends fixture on my local does not update the database. It's there a way I can manually run updates, I mean manually run 4.7.0-b1.php

@tsteur commented on November 22nd 2021 Member

@peterhashair you will also need to set the version number in the version class to 4.7.0-b1

@peterhashair commented on November 28th 2021 Contributor

Not sure if how we test the email templates on screenshot

@peterhashair commented on December 15th 2021 Contributor

the question I have for that one is are we missing an email Screenshot test, still thinking about how we implement this.

@tsteur commented on December 15th 2021 Member

@peterhashair do you want to test what the email looks like? In that case we could create an integration test and check that the created email content matches the expected HTML or text content. And basically not have a visual test for the email content.

@peterhashair commented on December 15th 2021 Contributor

@tsteur that make more sense, Will do a HTML code comparison

@github-actions[bot] commented on December 28th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions[bot] commented on January 6th 2022 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions[bot] commented on January 14th 2022 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@justinvelluppillai commented on January 18th 2022 Contributor

What's needed here to move this PR along @peterhashair? Can you update and request review?

@peterhashair commented on January 18th 2022 Contributor

@justinvelluppillai nothing needed here, just code review :)

@tsteur commented on January 19th 2022 Member
  • In the Invite User UI before the "Invite" button can we show some text what will happen after clicking on the button? Like we can mention that the user will receive an email, that the user has X days to confirm this email. And we can also inform them they can reject an unaccepted invite by doing xyz? What else would be important?

image

  • The email contains %s text
    image

  • Can we disable maybe the "Password" form field (maybe also in the API) until a user confirmed the invite? Currently a super user can set the password for a person who was invited but has not accepted anything yet. I checked it wasn't possible to log in but may be still good to prevent this?

  • I'm not sure but it looks like I was automatically logged in if after accepting the invite without having to confirm the password. Possible that this was the case because the super user set a password on my behalf but doesn't look like it. I tested it with another user as well and I did not have to configure any password I was directly logged in and could click around in the UI and it counted as accepted.
    Instead it was accepted that we're showing the user a screen where the user confirms the invite, configures password, and also we need to make sure that they'll know what their login is. They won't be automatically logged in. If a privacy or terms or imprint URL is configured in general settings then they would need to agree to the configured terms and policy to confirm the invite. And if an Imprint URL is configured, then we would need to show this URL too but they would not need to agree to it. This could be shown at the bottom.

I stopped for now with reviewing as it looks like there may be still some bigger things left to do

@peterhashair commented on January 20th 2022 Contributor

@tsteur will do an update. :)

@github-actions[bot] commented on February 8th 2022 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@peterhashair commented on February 9th 2022 Contributor

@sgiehl that one is not ready yet :) , but will do a update

@peterhashair commented on February 10th 2022 Contributor

Just a note, I think this PR will be put on pause for now, currently is on the transaction from angular to VUE. Once the Vue implementation is finished, will transfer it to VUE and fix the remaining task.

@github-actions[bot] commented on March 10th 2022 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@sgiehl commented on April 26th 2022 Member

closing in favor of #18868

This Pull Request was closed on April 26th 2022
Powered by GitHub Issue Mirror