@peterhashair opened this Pull Request on June 16th 2022 Contributor

Description:

Here is what's left:

  • Activity log should recognize "invite user" and "invite user accepted". (create plugin issue)

Review

@sgiehl commented on June 21st 2022 Member

@peterhashair I only had a very rough look through your changes (couple of minutes only). And without looking into details it's already clear, that your changes would not be approve-able.

  • The PR still does not fulfill the requirements of the original
    • user should be removed when he declines
    • privacy url should be shown if available (not only terms & conditions)
    • ...
  • Your PR updates screenshots that are unrelated (e.g. overlay, visitor map)
  • ....

Overall it looks a bit like you've only addressed half of the comments I left on the previous PR.

@peterhashair commented on June 23rd 2022 Contributor

@bx80 I believe, this time I update all the remaining list,

  • the status filter is added
  • add coupe more invite user API tests
  • config for the default expiration date
  • a task for deleting expired invite
  • wording change
  • permission update, admin and super admin use can invite, only the invited from admin and super admin can see their invites
  • remove pending status to expire in n days.
@bx80 commented on June 27th 2022 Contributor

I'm seeing the 'Resend invite' envelope action for all users on the list. Shouldn't this just be showing for users who have a pending invite?
Screenshot_20220627_132432

@peterhashair commented on June 27th 2022 Contributor

@bx80 yes, js error, updated

@bx80 commented on June 27th 2022 Contributor

If the token expiry days config setting is set to something other than the default 7, then the correct expiry timestamp is used, but the invite email message will still show 7 days. It looks like the UserInviteNotes translation has the 7 days text hard-coded.

@peterhashair commented on June 27th 2022 Contributor

@bx80 updated

@bx80 commented on June 27th 2022 Contributor

@peterhashair Looks like it needed another UI screenshot update for UsersManager_resend_popup.png - I've added it.

@bx80 commented on June 27th 2022 Contributor

@sgiehl Hopefully all the issues you raised on the previous PR and all the requirements mentioned on the original issue should now have been addressed :crossed_fingers:. This PR is ready for a final review when you get a chance. :slightly_smiling_face:

@sgiehl commented on June 28th 2022 Member

@peterhashair to get the UI tests working correctly you still need to update the version here:
https://github.com/matomo-org/matomo/blob/bd5571897f4e8408ba168b34754f4fff523894a7/core/Version.php#L24
Otherwise the update script won't be triggered when setting up the omnifxiture.

@peterhashair commented on June 29th 2022 Contributor
@sgiehl commented on June 29th 2022 Member

@peterhashair That's quite easy to explain. You changed the update script. It now renames the column invite_status to invite_token. While I'm not sure what that should be good for, that actually causes all existing user having an invite_token value of accept (default value of invite_status) after updating.

@sgiehl commented on June 30th 2022 Member

@peterhashair I had another look and directly pushed a couple of fixes, so we might be able to get this done by next week.
Will have another look tomorrow and check again the comments on the previous PR to see if everything I mentioned back then is fixed now.

@sgiehl commented on July 5th 2022 Member

I think I'm done with applying fixes & improvements here. I really hope I didn't miss anything else. There were a couple of bugs and regressions (introduced to existing methods). Also the usability was broken at some places. In addition the test coverage of that new feature was quite bad, so I've added some additional tests.

@bx80 Could you please do another review of that issue? Would be good to look through the code changes as well as doing some local testing to ensure the whole invite user process works as expected.

@peterhashair please have a look at all the changes I did as well and leave comments if it's unclear why something was changed.

@peterhashair commented on July 5th 2022 Contributor

@sgiehl had a look through, make sense, thanks for fixing my mess 🤙

This Pull Request was closed on July 8th 2022
Powered by GitHub Issue Mirror