Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

feat(publish): better error msg on needs email verif #795

Merged
merged 5 commits into from
Oct 24, 2019

Conversation

ashleygwilliams
Copy link
Contributor

fixes #320

@ashleygwilliams ashleygwilliams added feature Feature requests and suggestions status - needs review labels Oct 22, 2019
@ashleygwilliams ashleygwilliams added this to the 1.5.0 milestone Oct 22, 2019
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

tiny nit on messaging but lgtm

@ashleygwilliams
Copy link
Contributor Author

@EverlastingBugstopper is it better now? feels awk to me but i dunno what to update it to

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

Small maintainability questions/suggestions. If neither looks compelling to you, we can go ahead with merge!

@@ -122,6 +121,33 @@ fn build_and_publish_script(
Ok(())
}

fn error_msg(status: reqwest::StatusCode, text: String) -> String {
if text.contains("\"code\": 10034,") {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could parse the text into a json blob if we wanted and allow for pretty formatting of error codes (e.g. access error codes directly instead of doing text.contains()). With that, we could match on error code value and elegantly print out helpful messages specific to error codes (for code 10034 and beyond)!

That being said, there's no immediate case for json parsing and handling here--the text approach is appropriate for our current error handling needs.

}"#
.to_string();
let result = error_msg(status, text);
assert!(result.contains("https://dash.cloudflare.com"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we even make the error message above a string const and compare the actual result in this test to this string const?

@ashleygwilliams
Copy link
Contributor Author

@gabbifish i think the idea of actually parsing the error to make it prettier would be a great next step- i'll file an issue!

@ashleygwilliams ashleygwilliams merged commit f0f685c into master Oct 24, 2019
@delete-merged-branch delete-merged-branch bot deleted the verify-email-plz-k-thx branch October 24, 2019 21:16
@ashleygwilliams ashleygwilliams added changelog - feature and removed feature Feature requests and suggestions labels Oct 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a readable error message for API code 10034 - Please verify your email
3 participants