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

Fix #find_or_create_professor #10

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Fix #find_or_create_professor #10

merged 5 commits into from
Sep 1, 2023

Conversation

grmnlrt
Copy link
Contributor

@grmnlrt grmnlrt commented Sep 1, 2023

Edusign API now returns the following error when we try to find a professor who doesn't exist by email 👇

Error - cannot get professor by email [email protected]

I referenced this error in a new constant in Response::Error::CANNOT_GET_PROFESSOR_BY_EMAIL_ERROR_MESSAGE and stoped rising an error when the API return this message.

@grmnlrt grmnlrt self-assigned this Sep 1, 2023
@ssaunier ssaunier merged commit 11d8b2d into master Sep 1, 2023
1 check passed
@ssaunier ssaunier deleted the fix-create-professor branch September 1, 2023 07:56
Copy link
Member

@Eschults Eschults left a comment

Choose a reason for hiding this comment

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

👋 @grmnlrt you need to change the condition at the end of L234 otherwise we won't enter the rescue, right?

raise Response::Error, Response::Error::PROFESSOR_NOT_FOUND_ERROR_MESSAGE if response.message == "professor not found"

💡 Maybe we could use the existing PROFESSOR_NOT_FOUND_ERROR_MESSAGE instead of adding a new constant?

@ssaunier ssaunier restored the fix-create-professor branch September 1, 2023 07:56
@ssaunier
Copy link
Member

ssaunier commented Sep 1, 2023

Oops sorry :/

@ssaunier
Copy link
Member

ssaunier commented Sep 1, 2023

@grmnlrt your should be able to re-push on the branch and reopen the PR

@grmnlrt
Copy link
Contributor Author

grmnlrt commented Sep 1, 2023

I tried it locally with a binding.pry and I was entering the rescue properly🤔

I guess know when we make the request with a wrong email the status return by API is "error"

@grmnlrt
Copy link
Contributor Author

grmnlrt commented Sep 1, 2023

Just retried locally and the line 234 isn't reached, the error is raise at L233 so I think we should keep it like this

@Eschults
Copy link
Member

Eschults commented Sep 1, 2023

Right, this line in the api method ensures the raise and rescue, never mind 👌 there might be room for simplification 👉 #11 WDYT ❓

@Eschults Eschults deleted the fix-create-professor branch September 1, 2023 08:36
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.

3 participants