Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update invite user #19366

Merged
merged 95 commits into from Jul 8, 2022
Merged

update invite user #19366

merged 95 commits into from Jul 8, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jun 16, 2022 •
edited

Description:

Here is what's left:

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

Review

@peterhashair peterhashair added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Jun 21, 2022
@peterhashair peterhashair added this to the 4.12.0 milestone Jun 21, 2022
@peterhashair peterhashair requested a review from bx80 June 21, 2022 01:41
@sgiehl
Copy link
Member

sgiehl commented Jun 21, 2022

@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.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jun 21, 2022
Peter added 10 commits June 22, 2022 11:15
revert screenshot
0361e28 
revert screenshot
delete if user if declined
7367e25 
delete use if declined
add privacy
split templated into 3 parts
remove decline
65dcd36 
remove decline
add invited by column
e2358cb 
add invited by column
Revert "add invited by column"
0e1a5bd 
This reverts commit e2358cb.
Update UserRepository.php
7b7cfc2 
add view user display
add email tests and update admin email
0c51b9a 
add email tests and update admin email
update php cs
c56a143 
update php cs
add expire task and default setting
93a3c03 
add expire task and default setting
add api tests
d350616 
add api tests
correct pending user from last time
da64aac 
correct pending user from last time
@sgiehl sgiehl force-pushed the inviteuserfix branch 2 times, most recently from 9cc9481 to cb67dd8 Compare June 30, 2022 16:50
@sgiehl
Copy link
Member

sgiehl commented Jun 30, 2022

@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 sgiehl force-pushed the inviteuserfix branch 10 times, most recently from e709f76 to 763d48f Compare July 5, 2022 10:32
@sgiehl
Copy link
Member

sgiehl commented Jul 5, 2022

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
Copy link
Contributor Author

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

@sgiehl sgiehl requested a review from bx80 July 6, 2022 07:30
bx80
bx80 approved these changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've re-tested: inviting a user, accepting, declining, re-sending invites, trying to accept the first invite when a second invite has been sent, invite expiry, deleting an invited user, filtering by status, automatic removal of the invited user on decline, trying to accept an invite after it has been deleted, etc. I've also read through latest the code changes, which all make sense (the improved variable names are helpful). Ivesuggested a couple of very minor language tweaks, but other than that it's all looking solid and I can't see any issues 👍

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@sgiehl @bx80
Apply suggestions from code review
bbb808f 
Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
@sgiehl sgiehl modified the milestones: 4.12.0, 4.11.0 Jul 8, 2022
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jul 8, 2022
@sgiehl sgiehl dismissed their stale review July 8, 2022 09:32

outdated

@sgiehl sgiehl merged commit 5df7397 into next_release Jul 8, 2022
@sgiehl sgiehl deleted the inviteuserfix branch July 8, 2022 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl left review comments

@bx80 bx80 bx80 approved these changes

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.11.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@peterhashair @sgiehl @bx80 @diosmosis