Show invalid IP errors in network explorer search#3534
Conversation
Test results 12 files 12 suites 12m 8s ⏱️ Results for commit e5f2160. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 5.14.x #3534 +/- ##
=======================================
Coverage 61.28% 61.29%
=======================================
Files 610 610
Lines 44713 44724 +11
Branches 43 43
=======================================
+ Hits 27401 27412 +11
Misses 17302 17302
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
024ace1 to
4000480
Compare
There was a problem hiding this comment.
In order for the AJAX call on the frontend to know that an error happened, the view has to return an error HTTP status code, e.g.
def get(self, request, *_args, **_kwargs):
form = NetworkSearchForm(request.GET)
status = 200
if form.is_valid():
context = self.form_valid(form)
else:
context = self.form_invalid(form)
status = 400
return self.render_json_response(context, status=status)This will trigger the .fail(...) handler in the submitForm function
There was a problem hiding this comment.
Right now it shows "Search failed!" regardless of the error, but we could check for errors in the response, and then display a concatenated error instead of the fallback "Search failed!"
.fail(function (response) {
const errors = response?.responseJSON?.errors;
if (!errors) {
return notifyFail('Search failed!');
}
const errorMsg = Object.values(errors)
.flat()
.join(', ');
notifyFail(errorMsg);
})There was a problem hiding this comment.
Right now it shows "Search failed!" regardless of the error, but we could check for errors in the response, and then display a concatenated error instead of the fallback "Search failed!"
Yes, I think that would be great!
There was a problem hiding this comment.
Why is the branch pointing to 5.14.x and not master?
The code returns the error in the response, but the client JS can't react to an error without using an error http status code, e.g. 400 for Bad Request
In addition, the errors won't show up due to this if block:
nav/python/nav/web/templates/networkexplorer/base.html
Lines 17 to 23 in e984713
The only "bug fix" I did #3529 was to always render the if block contents, so the request.fail handler has somewhere to show the error in the dom :)
Because this is a bugfix to code that has not been change since the latest stable release, see the PR template ("For a patch/bugfix affecting the latest stable version, it should be based on that version's branch (..x). For a new feature or other additions, it should be based on master.") |
Simrayz
left a comment
There was a problem hiding this comment.
Looks great :) I can add the frontend changes if you want.
That would be great! I think it would be easiest to make a new pull request and have that with |
d32d0bb to
e5f2160
Compare
|
Scope and purpose
When reviewing #3529 I noticed that the bug that @Simrayz found goes actually even deeper than the frontend just showing when a search failed.
Because every time an invalid IP was given and exact search chosen that triggered an internal server error:
To reproduce:
Go to
/networkexplorer/, enterinvalidinto the search field and selectIPin the dropdown, select "Search exact (no substring)", the click "Search". See internal server error.The only thing this PR is lacking is a way for the frontend to show the errors that are being send in as context. Maybe @Simrayz can help me there?
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.
<major>.<minor>.x). For a new feature or other additions, it should be based onmaster.