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

Ensure export command does not run if authentication process fails #27

Merged
merged 6 commits into from
Jul 19, 2021

Conversation

prof-anis
Copy link
Contributor

@prof-anis prof-anis commented Jul 13, 2021

What are you trying to accomplish with this PR?

Ensure export command does not run if authentication process fails, limit export to only authenticated users.

Why did you want to accomplish this?

Currently, when a user attempts to export, he would be asked if he wants to export as a guest user or as an authenticated user. If he decides to export as an authenticated user, the login command is called . If the user is not successfully logged in, the export still continues however it treats the user as a guest. This PR stops the whole export process if such scenario occurs so users wont think they are authenticated while they actually are not.

How did you accomplish this?

Ensure the login command returns an exit code which is used to check if it run successfully in the export command.
Move the check for authenticated user into a task in the multitask function

What could go wrong?

none

ToDos

  • It is safe to rollback these changes should an error occur in production.
  • I have tested these changes.
  • There are automated tests for these changes.

@prof-anis prof-anis requested a review from bosunski July 13, 2021 20:32
@prof-anis prof-anis closed this Jul 13, 2021
@prof-anis prof-anis reopened this Jul 13, 2021
@prof-anis
Copy link
Contributor Author

I would be closing this PR now based on our latest decision to limit exporting notebooks to only registered users.

@prof-anis prof-anis closed this Jul 13, 2021
@prof-anis prof-anis reopened this Jul 13, 2021
Copy link
Contributor

@bosunski bosunski left a comment

Choose a reason for hiding this comment

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

LGTM!

@prof-anis prof-anis merged commit ce46463 into master Jul 19, 2021
@prof-anis prof-anis deleted the quick-fix branch July 23, 2021 20:08
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.

2 participants