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

[admin] Add "goto" in subnet #70

Merged

Conversation

purhan
Copy link
Contributor

@purhan purhan commented Nov 2, 2020

This is slightly different than the solution proposed in the issue discussions but I hope this one works out better 😄

Preview

Peek 2020-11-02 17-35

Closes #39

@purhan purhan marked this pull request as draft November 2, 2020 12:28
@TravisBuddy
Copy link

Travis tests have failed

Hey @purhan,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

4th Build

View build log

./run-qa-checks
Check JavaScript Linting

./openwisp_ipam/static/openwisp-ipam/js/ip-request.js is OK.

./openwisp_ipam/static/openwisp-ipam/js/subnet.js
 #1 Expected ';' and instead saw '}'.
    $(this).toggle($(this).text().toLowerCase().indexOf(searchQuery) > -1) // Line 65, Pos 87
travis_time:end:065f3678:start=1604320419605081272,finish=1604320419720555312,duration=115474040,event=script

6th Build

View build log

./run-qa-checks
Check JavaScript Linting

./openwisp_ipam/static/openwisp-ipam/js/ip-request.js is OK.

./openwisp_ipam/static/openwisp-ipam/js/subnet.js
 #1 Expected ';' and instead saw '}'.
    $(this).toggle($(this).text().toLowerCase().indexOf(searchQuery) > -1) // Line 65, Pos 87
TravisBuddy Request Identifier: a18b1fe0-1d0a-11eb-916b-c54e98fd930e

@atb00ker
Copy link
Member

atb00ker commented Nov 2, 2020

this doesn't search within the pages that have not yet been loaded.

Ideally, the search should filter all the IPs in the subnet. Loaded in frontend or not.

@purhan
Copy link
Contributor Author

purhan commented Nov 3, 2020

@atb00ker What would be the proper approach? The API's response is paginated so I'd still have to cycle through all the pages to search for matching queries. Wouldn't that be too slow?

@atb00ker
Copy link
Member

atb00ker commented Nov 3, 2020

@purhan
When user searches for something and presses enter key. A request is send to the server, which would send filtered records in turn.
That way you don't have to cycle through records in the frontend but the server would do that for you. 😄

What do you think? 😄

@purhan
Copy link
Contributor Author

purhan commented Nov 4, 2020

I tried parsing the search queries from the backend with some successful results but it'd still be very slow to parse results for larger search inputs. I think it's not a good idea to use search and filter here, so it's best to just implement the go-to approach as suggested in the original issue (i.e. jump to the search query in the original list itself) otherwise I'd likely abandon this task 😄. What do you think?

@purhan purhan changed the title [admin] Add search in subnet [admin] Add "goto" in subnet Nov 5, 2020
@purhan purhan force-pushed the issues/39-implement-search-in-subnet branch from 0c1a08a to 845fdf9 Compare November 5, 2020 18:34
@purhan
Copy link
Contributor Author

purhan commented Nov 5, 2020

new
@atb00ker this is what it looks like now. Please take a look.

@purhan purhan force-pushed the issues/39-implement-search-in-subnet branch from 845fdf9 to 1cf84c4 Compare November 5, 2020 18:42
@TravisBuddy
Copy link

Travis tests have failed

Hey @purhan,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

6th Build

View build log

./run-qa-checks
Check JavaScript Linting

./openwisp_ipam/static/openwisp-ipam/js/ip-request.js is OK.

./openwisp_ipam/static/openwisp-ipam/js/subnet.js
 #1 'async' was used before it was defined.
    async function validateIp(ip_address) { // Line 62, Pos 5
 #2 Expected ';' and instead saw 'function'.
    async function validateIp(ip_address) { // Line 62, Pos 10
 #3 Expected 'function' at column 5, not column 11.
    async function validateIp(ip_address) { // Line 62, Pos 11
 #4 Expected '{' and instead saw 'return'.
    if (ip_address === '') return true; // Line 63, Pos 32
 #5 Stopping. (38% scanned).
     // Line 63, Pos 32
TravisBuddy Request Identifier: 7f6ff8e0-1fad-11eb-bd2e-17a8c440c3de

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks a lot @purhan! I haven't had time to test it yet but will leave some comments on the code now.

openwisp_ipam/static/openwisp-ipam/js/subnet.js Outdated Show resolved Hide resolved
@purhan purhan force-pushed the issues/39-implement-search-in-subnet branch from 1cf84c4 to 09f1949 Compare November 6, 2020 07:43
@TravisBuddy
Copy link

Travis tests have failed

Hey @purhan,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

6th Build

View build log

./run-qa-checks
Check JavaScript Linting

./openwisp_ipam/static/openwisp-ipam/js/ip-request.js is OK.

./openwisp_ipam/static/openwisp-ipam/js/subnet.js
 #1 'isValid' was used before it was defined.
    validateIp(input, (isValid) => { // Line 81, Pos 28
 #2 Unexpected '('.
    validateIp(input, (isValid) => { // Line 81, Pos 27
 #3 Missing space between '=' and '>'.
    validateIp(input, (isValid) => { // Line 81, Pos 38
 #4 Unexpected '>'.
    validateIp(input, (isValid) => { // Line 81, Pos 38
 #5 Stopping. (49% scanned).
     // Line 81, Pos 40
TravisBuddy Request Identifier: 3d3215f0-2005-11eb-9dab-419bf2960c30

@purhan purhan force-pushed the issues/39-implement-search-in-subnet branch from 09f1949 to 2cbcbf4 Compare November 6, 2020 08:05
@coveralls
Copy link

coveralls commented Nov 6, 2020

Pull Request Test Coverage Report for Build 228

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.336%

Totals Coverage Status
Change from base Build 214: 0.0%
Covered Lines: 598
Relevant Lines: 602

💛 - Coveralls

@purhan purhan force-pushed the issues/39-implement-search-in-subnet branch from 2cbcbf4 to 4484da1 Compare November 6, 2020 08:23
@purhan purhan marked this pull request as ready for review November 6, 2020 08:26
@TravisBuddy
Copy link

Travis tests have failed

Hey @purhan,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

6th Build

View build log

./run-qa-checks
Check JavaScript Linting

./openwisp_ipam/static/openwisp-ipam/js/ip-request.js is OK.

./openwisp_ipam/static/openwisp-ipam/js/subnet.js is OK.

Running checks for openwisp_ipam
./openwisp_ipam/templates/admin/openwisp-ipam/subnet/change_form.html needs newline at the end
ERROR: Blank endline check failed!
SUCCESS: Migration name check on "./openwisp_ipam/migrations/" with migrations-to-ignore: 0 successful!
Skipped 1 files
SUCCESS: Isort check successful!
All done! ✨ 🍰 ✨
53 files would be left unchanged.
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
SUCCESS: Commit message check successful!
No changes detected in app 'openwisp_ipam'
SUCCESS: Migrations check successful!
TravisBuddy Request Identifier: 827707a0-200b-11eb-9dab-419bf2960c30

@purhan purhan force-pushed the issues/39-implement-search-in-subnet branch from 4484da1 to 2e06ccd Compare November 6, 2020 08:44
Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

No bugs decided to show themselves while testing! 😄
Thanks @purhan ,
LGTM! 👍

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks @purhan, testing some improvements to this in #72, please see my changes there, will merge after build is successful.

Please take a look at #71 and #73 which are some issues I found during testing this feature.

@openwisp openwisp locked as resolved and limited conversation to collaborators Nov 9, 2020
@nemesifier nemesifier merged commit 2e06ccd into openwisp:master Nov 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[admin] Implement "go to" in huge subnet UI
5 participants