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

Show detailed exception only when needed #230

Merged
merged 12 commits into from
Feb 9, 2024
Merged

Conversation

mkangia
Copy link
Contributor

@mkangia mkangia commented Feb 2, 2024

https://dimagi.atlassian.net/browse/SC-3354

raise for status raises an exception in case of authentication failures which leads to a full stacktrace shown.
Seems reasonable to limit showing the full stacktrace only for verbose (debug) mode.

Also, update error message to a better message to let user know what all to check for in case of authentication failures.

Please ensure that your CommCare HQ credentials are correct and auth-mode is passed as 'apikey'
if using API Key to authenticate. Also, verify that your account has the necessary permissions
to use commcare-export.

Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

I like that the stack trace is still accessible when in verbose mode, however there is a point from the Jira ticket that is still missing in the PR. Specifically, we need to add a more user friendly error message that can provide details on what the user can check to resolve the error. This could simply be the suggested error message as in the ticket:

“401 Client Error: Unauthorized for URL [URL]. Please ensure that your CommCare HQ username and API Key are accurate, and verify that your account has the necessary permissions to access the DET tool.”

@mkangia
Copy link
Contributor Author

mkangia commented Feb 2, 2024

Thanks for the feedback @zandre-eng

I totally missed that. I thought the request was to show specific error. I will update that now

Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

Looks good. One comment, although not blocking.

commcare_export/commcare_hq_client.py Outdated Show resolved Hide resolved
@mkangia mkangia force-pushed the mk/skip-stacktrace branch from 4a5b8db to 7c1d36e Compare February 8, 2024 20:19
logger.error(
f"#{e}. Please ensure that your CommCare HQ credentials are correct & valid for "
f"the auth-mode passed. Also, Verify that your account has the necessary "
f"permissions to access the DET tool."
Copy link
Contributor

Choose a reason for hiding this comment

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

("the DET tool" means "the data export tool tool".) But I'm wondering whether the user knows that Dimagi internally refers to this as "the DET". In help text this command refers to itself as "commcare-export" or "the commcare-export tool". I think this text might be more familiar to a user:

Suggested change
f"permissions to access the DET tool."
f"permissions to use commcare-export."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Thank you

5d99505

@mkangia mkangia merged commit 45f3c55 into master Feb 9, 2024
5 checks passed
@mkangia mkangia deleted the mk/skip-stacktrace branch February 9, 2024 17:45
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.

3 participants