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

Add copy link after token been sent for the user doesnt have email set up from design #19707

Merged
merged 114 commits into from Sep 30, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Sep 5, 2022

Description:

Fixes: #19625

Add copy link after the token has been sent for the user who doesn't have an email set up from design.

  • As per the design, add a popup to resend invites.
  • As suggested, add an additional database field for the link token, and make 2 option works.
  • Add a new API endpoint.
  • Add 2 new Icon, arrow left and arrow right
  • As suggested, when over HTTPS using js WriteText otherwise executeCommand('copy'), there is still a security risk in WriteText, when failed, show the whole link.

This problem needs to update docs as well.

Review

Peter added 5 commits August 26, 2022 12:50
add design copy link
# Conflicts:
#	plugins/CoreHome/vue/dist/CoreHome.umd.js
#	plugins/CoreHome/vue/dist/CoreHome.umd.min.js
This reverts commit bb081bd.
add link token
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 6, 2022
@peterhashair peterhashair added this to the 4.12.0 milestone Sep 6, 2022
@peterhashair peterhashair added the Needs Review For pull requests that need a code review. label Sep 6, 2022
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

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

I left a few screenshots and comments for you to get it conforming to spec in slack 👍🏽

@justinvelluppillai justinvelluppillai removed the Needs Review For pull requests that need a code review. label Sep 7, 2022
Peter and others added 5 commits September 7, 2022 14:18
peterhashair and others added 2 commits September 26, 2022 23:46
…19738)

* Adding the abililty to exclude specific sites from the Vue site selector component.

* Adding more time to make sure that the select is loaded before trying to search.

* Changed things to filter the site on the server side.

* Added a few new test cases.

* Increasing amount of time for test.

* Updated the omnifixture.

* Fixing logic error in view.

* Removed collation from omnifixture since it was causing issues.

* Switching omnifixture back to 4.x-dev version.

* Making just the bare essential changes to make local test fixtures build correctly.

* Updating screenshot of API list.

* revert OmniFixture-dump to 4.x-dev

revert OmniFixture-dump to 4.x-dev

Co-authored-by: Peter <peter@innocraft.com>
Peter added 5 commits September 27, 2022 14:26
# Conflicts:
#	tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png
merge 4.x and update copied success ui tests
rebase to next release
rebase to next release
@peterhashair peterhashair changed the base branch from 4.x-dev to next_release September 27, 2022 01:39
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

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

In safari the copied link shows up in the header when I click "Copy Link" (see screenshot):

Screen Shot 2022-09-30 at 12 23 26 PM

Also when I click Copy Link in the modal in chrome, the modal remains open but the Copy Link and Resend Invite buttons now do nothing when clicked.

@justinvelluppillai justinvelluppillai removed the Needs Review For pull requests that need a code review. label Sep 29, 2022
update safrai and enable copy success and copy again
@peterhashair
Copy link
Contributor Author

@justinvelluppillai should be fixed now.

Peter and others added 2 commits September 30, 2022 12:37
@bx80
Copy link
Contributor

bx80 commented Sep 30, 2022

This is my first look at this PR, it does work 👍 and I'm aware there have already been a lot of points raised in the discussion above, I'll try to avoid rehashing previously covered comments.

a) If I want to create a new user and send them their invite link on slack, then I'd probably end up doing this:

  1. Click 'Invite a new user' (goes to a screen labeled 'Add a new user')
  2. Click 'Invite User'.
  3. Enter my password, then 'Invitation is sent' is shown.
  4. Go back to the user list.
  5. Search for the newly invited user (what was their email again? Type it in...)
  6. Click on an envelope row icon ("but I don't want to email, I want to copy...")
  7. Click 'Copy link' on the popup.
  8. Enter my password again (second time in 30 seconds 🙁 )
  9. Finally the invite link is in the clipboard.

I did this a couple of times before I realized that the 'Resend Invite' link shown in step 3) would allow me to jump to step 7). So although I saw 'Resend Invite' link, it didn't register because I want to copy the link. I never wanted to send an invite in the first place, so wasn't intuitive.

Could we either rename this to 'Copy Invite Link' or duplicate the link so we have 'Resend invite' and 'Copy invite link'?

b) Once the 'copy link' button is used the buttons are disabled but it isn't obvious that they no longer work. If I accidentally copied something else into my clipboard and lost the copied link then clicking the 'Copy link' button again has no effect. Wouldn't it be better to hide the buttons altogether and/or replace them with a close button?

c) Minor cosmetic: The resend invite popup box seems to have a different font size from the normal popups? The password popup font is much smaller, it looks like part of a different application.

d) Minor point: It might be slightly clearer to say 'Link copied to the clipboard' rather than just 'Link copied'.

e) Having to enter the password twice in the space of 10 seconds isn't great, probably beyond the scope of this PR, but maybe we note this somewhere else for improvement, since we're adding password prompts to more places in the UI and risk increasing user frustration.

@peterhashair
Copy link
Contributor Author

@bx80 I agree.

e) Having to enter the password twice in the space of 10 seconds isn't great, probably beyond the scope of this PR, but maybe we note this somewhere else for improvement, since we're adding password prompts to more places in the UI and risk increasing user frustration.

I guess this has come to the design question. @Javi-Ormaechea

Minor point: It might be slightly clearer to say 'Link copied to the clipboard' rather than just 'Link copied'.

@bx80
Copy link
Contributor

bx80 commented Sep 30, 2022

e) Having to enter the password twice in the space of 10 seconds isn't great, probably beyond the scope of this PR, but maybe we note this somewhere else for improvement, since we're adding password prompts to more places in the UI and risk increasing user frustration.

I guess this has come to the design question.

We could possibly look at a system level solution - same as sudo won't prompt for your password over and over if you entered it in the last 5 minutes. At the moment being tasked with adding 10 new users and copying their invites would make the admin type their password 20 times in under 5 minutes, it might be secure but it's the sort of UX that causes the admin to change their password to 12345 out of frustration 🙂 I can log a RFC issue on this if you like?

@justinvelluppillai
Copy link
Contributor

@bx80 yes it'd be good to maybe create a separate issue for that. I agree it wasn't fun getting links or sending invites during testing 👍🏽

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 spent some time doing functional testing, apart from the multiple password entry annoyance which we can address elsewhere, it works as expected and allows an admin to get an invite link copied to the clipboard fairly quickly after creating the initial invite. I can't see any outstanding issues with the code.

I've pushed a couple of UI test screenshot updates, once the test builds complete without issues then I think this is ready to merge 👍

@bx80 bx80 merged commit 2f7c352 into next_release Sep 30, 2022
@bx80 bx80 deleted the m19625 branch September 30, 2022 18:15
@sgiehl
Copy link
Member

sgiehl commented Oct 5, 2022

@peterhashair There is no need to recreate pull requests when you want to target another branch. You can simply change the target branch of a PR on GitHub. Also it is important to rebase your changes to the new target branch.
This PR actually included some other PRs that had been merged to 4.x-dev. It doesn't hurt that those changes are now included in the release. But it's important to keep any eye on the changes your PRs include.
In general you should avoid closing PRs in order to create a new one afterwards. Unless the changes included in the PR are completely different it would be good to always use the same PR till it is merged. This ensures that all comments that were made can be directly seen. By closing and recreating a PR you actually kind of loose all comments done before, as no one will search if there might have been previous PRs.

@peterhashair
Copy link
Contributor Author

peterhashair commented Oct 5, 2022

@sgiehl yes, sorry. I confused myself there, after I recreate the PR, and release I can rebase the PR. Will avoid that in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show user invite link when inviting user
7 participants