Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Handle 404 errors in QueryComboBox #5139
base: production
Are you sure you want to change the base?
Handle 404 errors in QueryComboBox #5139
Changes from 7 commits
8b666b5
6939172
9faedc7
948c118
d53d25f
24eebd4
c18eefe
b6ee081
404e701
5f41486
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now, the code never reaches this catch block as the 404 response is not considered to be an error. There is no 404 dialog in RSS Export Feed anymore (context: #4955) and the QCBX instead displays the formatted invalid resource and pretends it as valid.
We can hit the catch block if we also throw an error when
status === Http.NOT_FOUND
in line 66 over here:specify7/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts
Lines 64 to 69 in 6a364e0
In which case, the error gets caught in QCBX, value gets cleared and a save blocker is set but it does not get detected as a change being made and so the save button is not enabled and the user won't know something changed. The save blocker is not visible until you click on the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after adding hijackBackboneAjax like described in #5139 (comment), it looks like the save blocker is set correctly and the save buttons turn red:
could you verify if you also get this behavior after making that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, save does get blocked correctly in the CO form. However, in a prod environment the page redirects to a 404 page, which doesn't solve the issue in #4955.
Save blocker still doesn't work correctly in RSS export feed although the values did get cleared
and the blocker is only visible after clicking on the field --- but it doesn't matter as in prod the page will still redirect to 404
Making the change here: #5139 (comment) along with wrapping
hijackBackboneAjax
over the resource fetch directly inresourceApi.ts
helps avoid the 404There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe if we use
hijackBackboneAjax
higher up the call stack as opposed to wrapping it overresource.fetch()
, it gets set as undefined before all chained promises are resolvedspecify7/specifyweb/frontend/js_src/lib/utils/ajax/backboneAjax.ts
Line 29 in 8d0052c
and then the
expectedErrors
over here is an empty list instead of[404]
and so it gets treated as an uncaught errorspecify7/specifyweb/frontend/js_src/lib/utils/ajax/response.ts
Lines 11 to 26 in 8d0052c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, sorry! I forgot that the behavior is different in production
You are correct
The cause of the issue is that rget is actual awaiting two promises:
specify7/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.ts
Lines 521 to 522 in 9faedc7
the assumption that
hijackBackboneAjax()
makes is that the variables it sets (expectedErrors
, among others) are going to be retrieved in the same cycle - but that is not the case as the rget has two promises, and the one we are interested in comes 2nd.The issue is that
hijackBackboneAjax()
is called concurrently by multiple query combo boxes (i.e accession), and so if accession resource is fetched earlier, thehijackBackboneAjax()
does cleanup afterward, unsettlingexpectedErrors
and other variables - before the collector request was sent out with correctexpectedErrors
In all other places where hijackBackboneAjax is used, it's used directly on
.fetch()
, so this issue is not present.two options:
we don't want to do too many changes to resourceApi.ts as we are hoping to do major refactor to it soon, but in an ideal world the change you made with introducing the "options" parameter would be made for all methods in resourceApi that make network requests.
for now, could you please make the following changes:
ajax()
directly, it will be able to easily pass on the options, without the need for hackyhijackBackboneAjax()