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(shorebird_cli): Add account create command #334

Merged
merged 18 commits into from
Apr 20, 2023

Conversation

bryanoltman
Copy link
Contributor

@bryanoltman bryanoltman commented Apr 20, 2023

Description

Adds the shorebird account create command to allow users to sign up for Shorebird.

Example of a user attempting to log in without an account and subsequently running shorebird account create:

Screenshot 2023-04-20 at 3 33 24 PM

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

logger.info('''
Shorebird is currently only open to trusted testers. To participate, you will need a Google account for authentication.

The first step is to sign in with a Google account. Please follow the sign-in link below:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow the link to sign-in with your Google account:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with "Follow the link below to authenticate:" because "your Google account" felt redundant after "Shorebird currently requires a Google account for authentication". lmk what you think

@bryanoltman bryanoltman marked this pull request as ready for review April 20, 2023 17:15
Co-authored-by: Felix Angelov <felix@shorebird.dev>
@@ -34,7 +34,7 @@ class CancelSubscriptionCommand extends ShorebirdCommand

final User user;
try {
user = await client.getCurrentUser();
user = (await client.getCurrentUser())!;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're already logged in we could cache the user and avoid having to make this extra request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow – we won't be running login or signUp in this command, and we'll definitely want to get the latest subscription info for this user before dealing with subscription info.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry I think I misread this. We might want to remove the auth.isAuthenticated check here and instead handle the case where getCurrentUser is null rather than force unwrapping though 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should still fail in the same way as before – if the user is null or if getCurrentUser fails in some other way, we log the error and exit. What's your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is mainly that the error from force unwrapping a null value won't be helpful to the end user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! That makes sense. Updated.

@@ -1,29 +0,0 @@
import 'dart:async';
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@bryanoltman bryanoltman requested a review from felangel April 20, 2023 20:03
bryanoltman and others added 3 commits April 20, 2023 16:13
Co-authored-by: Felix Angelov <felix@shorebird.dev>
Co-authored-by: Felix Angelov <felix@shorebird.dev>
@bryanoltman bryanoltman requested a review from felangel April 20, 2023 20:24
Copy link
Contributor

@felangel felangel left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@bryanoltman bryanoltman merged commit 69e8738 into main Apr 20, 2023
@bryanoltman bryanoltman deleted the bo/account-create-command branch April 20, 2023 20:28
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.

3 participants