Skip to content

Bugfix/defaultp not used #15

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

Merged
merged 28 commits into from
Mar 13, 2020
Merged

Bugfix/defaultp not used #15

merged 28 commits into from
Mar 13, 2020

Conversation

antazoey
Copy link
Contributor

@antazoey antazoey commented Mar 12, 2020

  • Begin dates now have to be in quotes when giving a time
  • Fix bug with not using the right profile when setting / resetting password and showing profile not showing right one
  • It now verifies your password before letting you set it
  • sets encoding for file loggers and open util function

@antazoey antazoey requested a review from alanag13 March 12, 2020 19:52
@@ -109,7 +111,12 @@ def set_profile(args):

def prompt_for_password_reset(args):
"""Securely prompts for your password and then stores it using keyring."""
password.set_password_from_prompt(args.profile_name)
profile = get_profile(args.profile_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will get the default profile if args.profile_name is None. Thus, profile.name will not be None anymore.

@@ -91,7 +92,8 @@ def show_profile(args):
print(u"\t* {0} = {1}".format(ConfigAccessor.USERNAME_KEY, profile.username))
print(u"\t* {0} = {1}".format(ConfigAccessor.AUTHORITY_KEY, profile.authority_url))
print(u"\t* {0} = {1}".format(ConfigAccessor.IGNORE_SSL_ERRORS_KEY, profile.ignore_ssl_error))
if password.get_password(args.profile_name) is not None:
profile_name = profile.name
if password.get_stored_password(profile_name) is not None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the reason show profile was inaccurate

@antazoey
Copy link
Contributor Author

@alanag13 I changed the dates like we discussed as well

new_password = password.get_password_from_prompt()
if not validate_connection(profile.authority_url, profile.username, new_password):
print_error(
"Failure to save password. Can't validate username, password, or authority URL."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unfortunate that this could go wrong for other reasons besides the password not working (until maybe we can distinguish that through custom exceptions?)

By other reasons, I mean like 500 errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you tried to make the message generic but that could be confusing for users in the most common case (the url, password, or username was wrong). I would say something like:
Your password was not saved because your credentials failed to validate. Check your network connection and the spelling of your username and server url.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks that works and still is generic enough

Copy link
Contributor

@alanag13 alanag13 left a comment

Choose a reason for hiding this comment

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

just adjust the error message for when the credentials are wrong

new_password = password.get_password_from_prompt()
if not validate_connection(profile.authority_url, profile.username, new_password):
print_error(
"Failure to save password. Can't validate username, password, or authority URL."
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you tried to make the message generic but that could be confusing for users in the most common case (the url, password, or username was wrong). I would say something like:
Your password was not saved because your credentials failed to validate. Check your network connection and the spelling of your username and server url.

@@ -63,24 +63,24 @@ def _verify_timestamp_order(min_timestamp, max_timestamp):
raise ValueError(u"Begin date cannot be after end date")


def _parse_timestamp(date_tuple):
def _parse_timestamp(date_and_time):
Copy link
Contributor

Choose a reason for hiding this comment

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

does this method only get called if a min or max timestamp is present? if not, it looks like there would be some problem if date_and_time was None

@@ -198,9 +207,10 @@ def _verify_args_for_set(args):
missing_values = not args.c42_username and not args.c42_authority_url
if missing_values:
try:
profile = get_profile(args.profile_name)
accessor = get_config_accessor()
Copy link
Contributor Author

@antazoey antazoey Mar 13, 2020

Choose a reason for hiding this comment

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

@alanag13 This fixed an issue where you would get two error messages when only one made sense during set.. kinda weird to be catching a SystemExit too.

@antazoey antazoey merged commit 0de1f29 into master Mar 13, 2020
@antazoey antazoey deleted the bugfix/defaultp-not-used branch March 13, 2020 16:51
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