Here is what's left:
@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.
Overall it looks a bit like you've only addressed half of the comments I left on the previous PR.
@bx80 I believe, this time I update all the remaining list,
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?
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 Looks like it needed another UI screenshot update for
UsersManager_resend_popup.png - I've added it.
@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:
@peterhashair to get the UI tests working correctly you still need to update the version here:
Otherwise the update script won't be triggered when setting up the omnifxiture.
@sgiehl try to do the reset password pending user check https://github.com/matomo-org/matomo/pull/19366/commits/2a61251978d1fa36fb2f8e9f98153ba7b4c06e66, but it keeps failing on of the UI test. https://app.travis-ci.com/github/matomo-org/matomo/jobs/575038221
@peterhashair That's quite easy to explain. You changed the update script. It now renames the column
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.
@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.
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.
@sgiehl had a look through, make sense, thanks for fixing my mess 🤙