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

feat(plg): remove seats from subscription #63408

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Conversation

taras-yemets
Copy link
Contributor

@taras-yemets taras-yemets commented Jun 20, 2024

Closes https://github.com/sourcegraph/sourcegraph/issues/63234
Design

Adds a button to remove the remaining invites from a subscription.

Screen.Recording.2024-06-21.at.01.20.23.mov

Known deviation from design (cc: @rrhyne)

The “Remove User” success message and the “Remove Seats from Subscription” alerts are positioned differently than in the design. The “Remove User” banner, along with other user and invite list alerts, is placed above the users list, below the page title or the invites textbox if rendered. The “Remove Seats from Subscription” banner, along with other invite textbox alerts, is located above the invites textbox instead of above the page title and all the page content. These issues will be addressed in follow-up PRs, as we need to refactor the existing Team Members page layout.
Created an issue to address it:

Test plan

  • Run a Sourcegraph instance in dotcom mode
  • Sign in as a team admin
  • Ensure there are free seats on your team (invites remaining)
  • Ensure "Remove invites from plan" button is rendered
  • Check the number of invites remaining
  • Click the button
  • Ensure the subscription has been updated and now the max seats reduced by the number of remaining invites
  • Ensure the success banner is rendered

Changelog

@cla-bot cla-bot bot added the cla-signed label Jun 20, 2024
@taras-yemets taras-yemets marked this pull request as ready for review June 20, 2024 22:32
@taras-yemets taras-yemets requested a review from a team June 20, 2024 22:33
@taras-yemets taras-yemets requested a review from rrhyne June 20, 2024 22:37
Copy link
Contributor

@chrsmith chrsmith left a comment

Choose a reason for hiding this comment

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

The code changes looks good. But I'm not sure about the UI design here. (But will defer to Rob, and admit I haven't been following the specific threads/follow ups closely.)

But it just seems a little unexpected to have a "remove all unused seats" button, which will have implications on the billing. (Since if the admin were to immediately "add one more seat" they would need to pay for that, even though they just got done removing that seat.)

So I would expect a more "heavyweight" sort of "+ / - button" approach, and have this on the modify Team Seats page. (But perhaps that's already the plan. Or we have good reason to go with this approach.)

Send
</ButtonLink>
</Button>
<Button
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, if there are "no unfilled seats" this element won't be visible at all right? (Since, it wouldn't make sense to offer the user to send invites to fill seats that don't exist?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taras-yemets
Copy link
Contributor Author

But it just seems a little unexpected to have a "remove all unused seats" button, which will have implications on the billing. (Since if the admin were to immediately "add one more seat" they would need to pay for that, even though they just got done removing that seat.)

I agree with this concern. We need to be more explicit about what exactly is going to happen when we free N seats and then suddenly want them back in the next moment.
cc: @rrhyne.

@taras-yemets
Copy link
Contributor Author

@chrsmith, @rrhyne, I'm going to merge it as it is and iterate in the follow-up PRs.

@taras-yemets taras-yemets merged commit 2c2ec72 into main Jun 20, 2024
11 checks passed
@taras-yemets taras-yemets deleted the ty/plg-remove-seats branch June 20, 2024 23:35
@rrhyne
Copy link
Contributor

rrhyne commented Jun 21, 2024

Thanks Taras... good call merging early. Would definitely like consistency on where banners are displayed.

This all worked really well in my testing.

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

Successfully merging this pull request may close these issues.

Allow for team seat count decreases
3 participants