Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Login with option to agree to terms of service and human friendly errors #866
Conversation
added some commits
Oct 12, 2016
|
The static checks and the tests have passed now. The relevant change needed in the server side has also landed. So if the review is OK, this can land. |
sergiusens
requested changes
Oct 25, 2016
Hi, thanks for the PR, looks generally good. I did a first pass of this already and have some changes requested
| + return None | ||
| + | ||
| + | ||
| +def _check_dev_agreement_and_namespace_statuses(store): |
sergiusens
Oct 25, 2016
Collaborator
Can we just raise exceptions if the conditions are not met. The exception would carry a friendly error message we would raise to the user just like we do for all others
psivaa
Nov 1, 2016
Contributor
This has been taken on board. Added an exception type for this, with a human friendly error message.
| + storeapi.constants.NAMESPACE_ERROR.format(url)) | ||
| + else: | ||
| + raise | ||
| + return True |
| + raise | ||
| + return True | ||
| + | ||
| + | ||
| def _login(store, acls=None, save=True): |
sergiusens
Oct 25, 2016
Collaborator
I don't mind if this just raises an exception that is friendly enough and this return _login(store) is refactored a bit.
This may not be a task for you and you may not want to do it, but we have a lot of double logic with exceptions and if True/False around.
psivaa
Nov 1, 2016
Contributor
I personally don't mind doing this. But moving the exception handling to login() for refactoring return _login(store) will imho shift the complexity to login. So i don't feel there is much gain here. But if you still insist on it, i can happily do that :)
| - return False | ||
| + | ||
| + # Continue if agreement and namespace conditions are met. | ||
| + if not _check_dev_agreement_and_namespace_statuses(store): |
sergiusens
Oct 25, 2016
Collaborator
just deal with the exception in the exception catching logic a couple lines below
| + | ||
| + if not response.ok: | ||
| + raise errors.DeveloperAgreementSignError( | ||
| + 'Failed to sign developer ToS.') |
sergiusens
Oct 25, 2016
Collaborator
Hmm, response.ok not being ok may not be an indication of failing to sign the developer agreement and a 503 instead
psivaa
Nov 1, 2016
Contributor
Right. The error message was just an indicator. Now use an exception to use the response itself.
| @@ -87,20 +87,32 @@ class StoreMacaroonNeedsRefreshError(StoreError): | ||
| fmt = 'Authentication macaroon needs to be refreshed.' | ||
| +class DeveloperAgreementSignError(StoreError): | ||
| + | ||
| + fmt = 'Developer agreement sign error: {message}' |
| + int(self.headers['Content-Length'])).decode('utf8') | ||
| + data = json.loads(string_data) | ||
| + if data['latest_tos_accepted'] is not True: | ||
| + self.send_response(400) |
|
Would you mind please updating the PR title to be more descriptive? The bug reference should be in the PR description and the commit, not the title. |
psivaa
changed the title from
Bug/1619193
to
login_with_transparent_terms_signing_errors
Nov 1, 2016
psivaa
changed the title from
login_with_transparent_terms_signing_errors
to
Login with option to agree to terms of service and human friendly errors
Nov 1, 2016
|
Thanks for the reviews. I've addressed them and replied inline. Would appreciate a re-review please. |
| + except storeapi.errors.StoreAccountInformationError: | ||
| + return _fail_login(storeapi.constants.ACCOUNT_INFORMATION_ERROR) | ||
| + except storeapi.errors.NeedTermsSignedError as e: | ||
| + return _fail_login(e.message) |
psivaa
Nov 3, 2016
Contributor
The NeedTermsSignedError will contain different messages based on the error in _check_dev_agreement_and_namespace_statuses.
| + 'alongside the other details for your snap. Please visit {} and login ' | ||
| + 'again.') | ||
| +AGREEMENT_INPUT_MSG = ( | ||
| + 'Do you agree to the developer terms and conditions. ({})? [y/N]') |
josepht
Nov 3, 2016
Contributor
I'm not a fan of mixing formatting in the constants. It's not a blocker for me just an opinion.
psivaa
Nov 3, 2016
Contributor
Yea, I agree that its not perfect. But I think keeping them here will make the actual logic more readable.
| +class DeveloperAgreementSignError(StoreError): | ||
| + | ||
| + fmt = ( | ||
| + 'There was an error whilst signing developer agreement.\n' |
and others
added some commits
Nov 3, 2016
All checks have passed
| @@ -286,6 +286,9 @@ def push_validation(self, snap_id, assertion): | ||
| def get_validations(self, snap_id): | ||
| return self.sca.get_validations(snap_id) | ||
| + def sign_developer_agreement(self, latest_tos_accepted=True): |
| @@ -618,6 +621,19 @@ def close_channels(self, snap_id, channel_names): | ||
| response.content)) | ||
| raise errors.StoreChannelClosingError(response) | ||
| + def sign_developer_agreement(self, latest_tos_accepted=True): |
added some commits
Nov 14, 2016
Some checks were not successful
|
Thanks for the comments. Pushed the fix for them. |
sergiusens
merged commit 36d7765
into
snapcore:master
Nov 14, 2016
3 of 5 checks passed
psivaa
deleted the
psivaa:bug/1619193
branch
Nov 15, 2016
psivaa
restored the
psivaa:bug/1619193
branch
Nov 15, 2016
psivaa
deleted the
psivaa:bug/1619193
branch
Nov 15, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
psivaa commentedOct 13, 2016
•
Edited 1 time
-
psivaa
Oct 13, 2016
Storeside changes to introduce graceful failure messages and an option to sign developer agreement via snapcraft CLI. Also includes some refactoring in the surrounding area to move messages/errors/warnings into 'storeapi/constants.py'
LP: #1619193