@peterhashair opened this Pull Request on March 2nd 2022 Contributor

Description:

fixes: https://github.com/matomo-org/matomo/issues/13321

Since we move UsersManager to VUE. Redo the entire PR in the VUE version and fixes the remaining comments

This PR is VUE version of https://github.com/matomo-org/matomo/pull/18351

This probably needs docs for it, still working on the docs, not sure we want people inject their only term & condition on the set password page.

Changes

add one table column

  • invite_at datetime

Review

@peterhashair commented on March 4th 2022 Contributor

@sgiehl I got a question for the structure, is that ok for me to move some of the API.php private methods to a different class, just the public method remains in the API.php. In this case, is the UsersManager/API.php

@sgiehl commented on March 7th 2022 Member

@sgiehl I got a question for the structure, is that ok for me to move some of the API.php private methods to a different class, just the public method remains in the API.php. In this case, is the UsersManager/API.php

If private API methods could be reused somewhere else, it makes sense to move them to another class.

@peterhashair commented on April 21st 2022 Contributor

@diosmosis sorry to bother, just checking for the Build Vue, I keep getting errors from plugin MobileMessaging any idea, what causing that one?

@diosmosis commented on April 21st 2022 Member

@peterhashair ScheduledReports isn't in MobileMessaging's umd.metadata.json so it's not being built first. Rebuilding locally adds it back for me. Or you can add it manually, the file should look like:

{
  "dependsOn": [
    "CoreHome",
    "CorePluginsAdmin",
    "ScheduledReports"
  ]
}

I'm not sure why it isn't there, though.

@peterhashair commented on June 2nd 2022 Contributor

@bx80 fixed it.

@bx80 commented on June 8th 2022 Contributor

Thanks for that @sgiehl, you are right - there are several issues here that should have been caught during review and will need to be addressed with a fix PR.

With the auth token issue, what do you think about adding a boolean flag invite_token to the user_token_auth table to indicate that the token is only valid for user invitation, similar to the existing system_token option? Any attempt to use an invite_token for general API access could then be rejected in addition to removing the token entirely once the invite is accepted / rejected?

@sgiehl commented on June 8th 2022 Member

@bx80 using the token auth table for that might not have been the best choice, as it will require to change all the existing code, that by default when checking for tokens it will need to ignore such special tokens.

Personally I might have added two fields to the user table. invite_token and invite_expire
I'm not sure if we would need a status field at all.
If someone is invited, the generated token will be stored in invite_token, together with the expire datetime in invite_expire.
Every user where both fields are filled is in the state pending.
If the expire datetime passes, we can show expired as status.
If a user declines the request, the requirements actually defined, that the user should be removed, to avoid storing any personal data, so this status isn't needed.
If a user accepts the invite, both invite fields would be cleared and such users will have the status active.

@peterhashair commented on June 8th 2022 Contributor

@sgiehl that makes sense, I will rework the table structure on that one.

This Pull Request was closed on June 6th 2022
Powered by GitHub Issue Mirror