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

Irregularity in errorCallback in backend-api services #9982

Closed
nishantwrp opened this issue Jul 20, 2020 · 10 comments
Closed

Irregularity in errorCallback in backend-api services #9982

nishantwrp opened this issue Jul 20, 2020 · 10 comments

Comments

@nishantwrp
Copy link
Contributor

nishantwrp commented Jul 20, 2020

For example

(error) => {
if (errorCallback) {
errorCallback(error.statusText);
}

(error) => {
if (errorCallback) {
errorCallback(error);
}

}, function(errorResponse) {
if (errorCallback) {
errorCallback(errorResponse.data);
}

}, function(errorResponse) {
ctrl.setStatusMessage(
'Server error: ' + errorResponse.data.error);
}

}, (errorResponse) => {
if (errorCallback) {
errorCallback(errorResponse.error);
}
});

It can be seen that there are different kind of properties are passed to the errorCallback. I think that some of them may be wrong. Also shouldn't the thing passed to errorCallback in a http request be the same?

/cc @seanlip @vojtechjelinek @ankita240796

@seanlip
Copy link
Member

seanlip commented Jul 20, 2020

Yes, it would probably be good to standardize this. (Though some investigation is probably needed to determine what to standardize it to.)

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 24, 2020

Screenshot_20200724_161726-1

Here is what the errorResponse looks like.

Looking at it, imo we should standardise it to either errorResponse.statusText or errorResponse.error.error. I'm slightly in favour of errorResponse.error.error because it displays the exact error. In this case it is I don't want it to work because I put in

raise Exception('I don\'t want it to work')

in the backend file.

/cc @seanlip @vojtechjelinek

@seanlip
Copy link
Member

seanlip commented Jul 25, 2020

Hi @nishantwrp -- how about errorResponse.data.error? Maybe analyze the other possibilities as well?

@seanlip
Copy link
Member

seanlip commented Jul 25, 2020

(Do a full comparison of all the options and then we can give you advice on how to proceed.)

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 25, 2020

Screenshot_20200726_015551

Hi @seanlip, Actually in http requests made by $http angular Js client for http clients the error is of above format. So, in those files we can use errorResponse.data.error.

And in angular files. In the http requests made by HttpClient we can use errorResponse.error.error..

@seanlip
Copy link
Member

seanlip commented Jul 25, 2020

Oh, I see. Yup, that convention sounds fine to me, thanks! I suggest that you coordinate with the Angular team (@bansalnitish @srijanreddy98 @orthodoxparadox) so that they know the convention too and can enforce it while migrating new services.

@sainanee
Copy link
Contributor

sainanee commented Jul 25, 2020

@seanlip Should I add it to the Migration Guide?

@seanlip
Copy link
Member

seanlip commented Jul 25, 2020

Yup, probably a good idea -- thanks!

@sainanee
Copy link
Contributor

sainanee commented Jul 25, 2020

Done! Added to point 6 c) in migrating services in the Angular Migration Guide!

nishantwrp added a commit that referenced this issue Jul 27, 2020
…ove remaining any types (#10054)

* Refactor backend api services and remove remaining any types

* Remove testing code

* Revert unnecessary change

* Eslint check

* reviews

* reviews

* Update invalid_eslint_any_type.ts

* backend tests
shavavo pushed a commit to shavavo/oppia that referenced this issue Aug 20, 2020
…d remove remaining any types (oppia#10054)

* Refactor backend api services and remove remaining any types

* Remove testing code

* Revert unnecessary change

* Eslint check

* reviews

* reviews

* Update invalid_eslint_any_type.ts

* backend tests
@kevintab95 kevintab95 added this to Awaiting Triage in Learner and Creator Experience Aug 28, 2020
@kevintab95 kevintab95 moved this from Awaiting Triage to Learner Pages (@aks681) in Learner and Creator Experience Aug 30, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Feb 9, 2021

Fixed by #10054. /cc @srijanreddy98 to remind you about this convention.

Learner and Creator Experience automation moved this from Lesson-Related Learner Pages (@aks681) to Done Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants