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

Studio: Allow blocking user from adding demo sites #354

Merged
merged 20 commits into from
Jul 26, 2024

Conversation

katinthehatsite
Copy link
Contributor

@katinthehatsite katinthehatsite commented Jul 11, 2024

Related to https://github.com/Automattic/dotcom-forge/issues/7370

Proposed Changes

This PR implements the UI for when the user is blocked from creating a demo site:

Screenshot 2024-07-11 at 9 10 57 PM

Testing Instructions

  • Navigate to the User Report Card for your own user
  • Use the Disable site creation button (at the bottom of the User Report Card) to block your user from creating demo sites
  • Pull the changes from this branch
  • Start the app with nvm use && npm install && npm start
  • Make sure to log in with the user that you just blocked from creating sites
  • Navigate to Share tab
  • Confirm that Add demo site button is disabled
  • Hover over the button and confirm that it displays a message about demo sites being disabled for this account

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@katinthehatsite katinthehatsite self-assigned this Jul 11, 2024
@katinthehatsite katinthehatsite requested a review from a team July 11, 2024 19:17
@wojtekn
Copy link
Contributor

wojtekn commented Jul 12, 2024

@katinthehatsite could we also account for a case in which we disable demo sites for the user who has Studio already open? Currently, when such a user tries to add a demo site, they will get a proper error message, but the button won't be disabled:

Screenshot 2024-07-12 at 12 22 03

@katinthehatsite
Copy link
Contributor Author

Hmm let me check what is happening there 👀

@katinthehatsite
Copy link
Contributor Author

@wojtekn I made some updates, would you be able to give it another try?

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I noticed that if the user being blocked already has demo sites created, the Update demo site button is enabled. I'd apply the same approach we've done for the Add button there.

Screenshot 2024-07-18 at 10 56 51

Additionally, when updating a demo site, I think the error message for blocked users should mention the failure reason

Screenshot 2024-07-18 at 10 37 17

src/components/content-tab-snapshots.tsx Outdated Show resolved Hide resolved
src/hooks/use-snapshots.tsx Show resolved Hide resolved
src/components/content-tab-snapshots.tsx Outdated Show resolved Hide resolved
src/components/content-tab-snapshots.tsx Outdated Show resolved Hide resolved
@katinthehatsite
Copy link
Contributor Author

Thanks for the review @fluiddot ! I made some changes to block the Update site button when the user is blocked in 0b95433

I tried the alternative solution but I think at the end, it comes to the same thing that @wojtekn mentioned initially with the app not updating the data in real-time. I left it as is for now, let's wait for some comments and see if we should change the approach of polling the endpoint.

Copy link
Member

@sejas sejas 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 created a PR to simplify the logic of the update demo site button. Let me know what do you think. #388

I've tested login in, loging out, enabling and disabling my user RC setting, and it works as expected. Great job!

Uwi625.mp4

From my point of view I would avoid adding the active listening. I see it as a corner case.
I like how we refresh the state when the user starts the app or logs in. We should use a similar criteria as we do with the quota of demo sites.

Instead of adding an active listening, I would disable the button after the first fail. Not a strong opinion.

src/components/content-tab-snapshots.tsx Outdated Show resolved Hide resolved
src/components/content-tab-snapshots.tsx Outdated Show resolved Hide resolved
src/components/content-tab-snapshots.tsx Show resolved Hide resolved
src/hooks/use-snapshots.tsx Show resolved Hide resolved
src/components/content-tab-snapshots.tsx Outdated Show resolved Hide resolved
@katinthehatsite
Copy link
Contributor Author

From my point of view I would avoid adding the active listening. I see it as a corner case.
I like how we refresh the state when the user starts the app or logs in. We should use a similar criteria as we do with the quota of demo sites.

Based on our discussions of this issue, I have implemented the behaviour mentioned in the comment above. Since it is more of an edge case, I would be okay not to poll the endpoint and update the information in real-time.

@sejas could you please give it another test and let me know what you think? Thanks!

Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

Kat!, thanks for applying the suggested cahnges.

I've tested the edge case when a user gets blocked and it has Studio running.
As shown in the video, the create demo site will fail and immediately will see the tooltip 👍 .

zo12GS.mp4

@katinthehatsite katinthehatsite merged commit 96a2b46 into trunk Jul 26, 2024
10 checks passed
@katinthehatsite katinthehatsite deleted the add/ui-for-when-user-is-blocked branch July 26, 2024 10:55
@fluiddot
Copy link
Contributor

Thanks for the review @fluiddot ! I made some changes to block the Update site button when the user is blocked in 0b95433

Thanks @katinthehatsite for addressing the comments I shared. I've tested the changes in trunk and confirmed it worked as expected 🎊 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants