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

Drop down menus will Break if their are less than six elements #8035

Closed
1 task
someRandomUser12 opened this issue May 8, 2020 · 27 comments
Closed
1 task
Assignees

Comments

@someRandomUser12
Copy link

Please confirm you have done the following before posting your bug report:

Describe the bug

Drop-down menus will display items for a period of time until it shows "The results could not be Loaded". This will prevent all drop down menus from work for a short period of time.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Login to the Demo on snipe-it website '
  2. Click on '"Create New" -> Asset . Click on Company Drop-down list '
  3. Scroll down to 'the bottom until you see "Loading More Results" and wait for 10 to 15 seconds'
  4. See error 'Menu items disappear and "The Results could not be loaded" appears. This will also break all other drop down menus on the app for a short period of time'.

Screenshots
error

Server (please complete the following information):

  • Snipe-IT Version Master(v4.9.2)(DEMO)
  • OS: [unknown]
  • Web Server: [unknown]
  • PHP Version

Desktop (please complete the following information):

  • OS: [Windows 10]
  • Browser [Chrome]
  • Version [latest]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]
@raychar
Copy link

raychar commented May 11, 2020

I found this was because the browser send too many /selectlist?page= requests in a given amount of time.

Reference:
https://github.com/ParadoxGuitarist/jamf2snipe/issues/9
https://snipe-it.readme.io/reference#api-throttling

But I've no idea about why this happend, anybody can explain this?

Snipe-IT Documentation
Snipe-IT is a free, open source IT asset management system. Features include management of assets, users, licenses, accessories, consumables and components, as well as two-factor authentication, LDAP/AD syncing, and asset acceptance confirmation.

@coaxke
Copy link

coaxke commented May 12, 2020

Also noticed this issue today after updating to the latest version in master and attempting to create a new Asset Model - I do have more than 6 items in a drop-down.
The issue appears to happen when I scroll to the bottom of a drop-down where Snipe-It would typically lazy-load more items:

image

This issue can be repro'd on the SnipeIT Demo site as well

@coaxke
Copy link

coaxke commented May 13, 2020

I reverted back to an older version of all.js and it fixed the problem - it seems like the issue comes from select2.

image

The existing scroll callback works fine while the new one fails with the loop described above. I was able to short-cut this problem by hack fixing all.js from

}), this.$results.on("scroll", this.loadMoreIfNeeded.bind(this)) }, e.prototype.loadMoreIfNeeded = function() { var e = t.contains(document.documentElement, this.$loadingMore[0]); !this.loading && e && this.$results.offset().top + this.$results.outerHeight(!1) + 50 >= this.$loadingMore.offset().top + this.$loadingMore.outerHeight(!1) && this.loadMore() }, e.prototype.loadMore = function() {

to

}), this.$results.on("scroll", function() { var e = t.contains(document.documentElement, this.$loadingMore[0]); !this.loading && e && this.$results.offset().top + this.$results.outerHeight(!1) + 50 >= this.$loadingMore.offset().top + this.$loadingMore.outerHeight(!1) && this.loadMore() }) }, e.prototype.loadMore = function() {

I hope this makes sense - I'm certainly not a JS dev as you can probably tell :)

Update: Pull down latest from master - this issue is now fixed.

@raychar
Copy link

raychar commented May 13, 2020

I reverted back to an older version of all.js and it fixed the problem - it seems like the issue comes from select2.

image

The existing scroll callback works fine while the new one fails with the loop described above. I was able to short-cut this problem by hack fixing all.js from

}), this.$results.on("scroll", this.loadMoreIfNeeded.bind(this)) }, e.prototype.loadMoreIfNeeded = function() { var e = t.contains(document.documentElement, this.$loadingMore[0]); !this.loading && e && this.$results.offset().top + this.$results.outerHeight(!1) + 50 >= this.$loadingMore.offset().top + this.$loadingMore.outerHeight(!1) && this.loadMore() }, e.prototype.loadMore = function() {

to

}), this.$results.on("scroll", function() { var e = t.contains(document.documentElement, this.$loadingMore[0]); !this.loading && e && this.$results.offset().top + this.$results.outerHeight(!1) + 50 >= this.$loadingMore.offset().top + this.$loadingMore.outerHeight(!1) && this.loadMore() }) }, e.prototype.loadMore = function() {

I hope this makes sense - I'm certainly not a JS dev as you can probably tell :)

it works! thank u!

@coaxke
Copy link

coaxke commented May 17, 2020

Looks like snipe has noted the select2 fix on #7997 so it must be in-flight :)

@Flheart2k20
Copy link

Hello,

Exact same issue here with 405 method not allowed errors. Waiting for the fix.

@Flheart2k20
Copy link

Flheart2k20 commented May 24, 2020

The all.js edit workaround works for me too.
Nice finding Coaxke!

@ardvw
Copy link

ardvw commented May 26, 2020

I've also edited the all.js file. However, it doesn't seem to be working in my environment. To be clear, we are talking about the all.js file in the ./public/js/build directory right?

Or do I need for each dropdown menu at least six items?

EDIT: I've changed the API limit from 120 to 500, so now I can edit and add a little bit more.

@Flheart2k20
Copy link

Hello ardvw,

you need to edit the all.js under the "dist" sub-container" if I remember well.

Cheers,

@jackwang-github
Copy link

I reverted back to an older version of all.js and it fixed the problem - it seems like the issue comes from select2.
image
The existing scroll callback works fine while the new one fails with the loop described above. I was able to short-cut this problem by hack fixing all.js from
}), this.$results.on("scroll", this.loadMoreIfNeeded.bind(this)) }, e.prototype.loadMoreIfNeeded = function() { var e = t.contains(document.documentElement, this.$loadingMore[0]); !this.loading && e && this.$results.offset().top + this.$results.outerHeight(!1) + 50 >= this.$loadingMore.offset().top + this.$loadingMore.outerHeight(!1) && this.loadMore() }, e.prototype.loadMore = function() {
to
}), this.$results.on("scroll", function() { var e = t.contains(document.documentElement, this.$loadingMore[0]); !this.loading && e && this.$results.offset().top + this.$results.outerHeight(!1) + 50 >= this.$loadingMore.offset().top + this.$loadingMore.outerHeight(!1) && this.loadMore() }) }, e.prototype.loadMore = function() {
I hope this makes sense - I'm certainly not a JS dev as you can probably tell :)

it works! thank u!

@MrMontesa
Copy link

Same here also running into the empty drop down issue. The all.js seems compressed (no newlines, etc), so had a hard time implementing the workaround. Any recommendations on how to patch this?
In the meantime I increased the API threshold, in app/Https/Kernel.php which helped a bit.
Could this be fixed in git, so that git pull will fix it?
Thanks

@snipe
Copy link
Owner

snipe commented Jul 7, 2020

Exact same issue here with 405 method not allowed errors. Waiting for the fix.

This would not be causing "method not allowed" issues.

I'm legit a little baffled why our hosted customers aren't seeing this.

@snipe
Copy link
Owner

snipe commented Jul 7, 2020

I'm still unable to reproduce this on the demo, but I definitely appreciate all the sleuthing here. I'm trying to see if bumping up our select2 library might help, but since I can't reproduce it, it makes it hard to tell. :-/

@snipe
Copy link
Owner

snipe commented Jul 7, 2020

Looks like select2 hasn't released anything new recently, so their latest is the same version we're running. :(

We can't edit those libraries manually or they'll just get overwritten next time we update things.

Looks like this bit of select2 might be what's causing the issue:

https://github.com/select2/select2/blob/e5131d0cc8dffd98ba36a68f3d027bf79e763cb4/src/js/select2/dropdown/infiniteScroll.js#L43-L61

select2/select2@d926025#diff-6bfa161cfa52b7188ae7e42328aaadbe

Screen Shot 2020-07-07 at 9 00 20 AM

GitHub
Select2 is a jQuery based replacement for select boxes. It supports searching, remote data sets, and infinite scrolling of results. - select2/select2

@coaxke
Copy link

coaxke commented Jul 7, 2020

That's so weird that the issue is no reproducible 😮 ; I just tried again in Demo on two browsers (no add-ins enabled) and could trigger it when attempting to create a new asset, selecting the model drop-down and scrolling to the bottom of the list (if dev tools are open you'll see a ton of requests).

@snipe
Copy link
Owner

snipe commented Jul 8, 2020

Unfortunately if I try to downgrade select2 from before that patch I referenced above, we'll lose the accessibility fixes that were added afterwards. I need to figure out how best to approach this.

Has anyone posted an issue on the select2 GH issues about this?

@coaxke
Copy link

coaxke commented Jul 9, 2020

I didn't raise an issue, sorry - I'm not a competent front-end dev and don't think I can explain the issue very well 😂

@snipe
Copy link
Owner

snipe commented Jul 10, 2020

I'm going to tag in @uberbrady here to see if he can maybe offer a PR to select2, or find another workaround.

@snipe
Copy link
Owner

snipe commented Jul 13, 2020

Well, the good news (ugh, sort of?) is that we're starting to see reports of this from a handful of customers as well, which gives us a little more to work with.

@uberbrady
Copy link
Collaborator

Okay, I can see what's going on here.

Snipe-IT is correctly returning {"pagination": {"more": false, ... in our response, but the Select2 JS is not respecting thefalse result correctly. It should stop requesting new pages.

Furthermore, if you were to have around 117 pages of results (the number where it broke for me during my testing), the front-end JS code should be more resilient to HTTP errors such as these - I was thinking about maybe implementing some kind of exponential back-off. The Select2 list slowing down a little bit is much better than it completely breaking.

I'll dig further into the documentation for Select2 and update this issue with my plan(s) of attack.

@uberbrady
Copy link
Collaborator

Nope, it's not them - it's us.

We use a /selectlist URL to back all of our Select2 select lists. Each of those methods - which live on all of the main controllers we use - ends up returning results through a SelectlistTransformer to get output. But in our Select2 javascript initialization, we also specify a processResults callback element, which transforms the JSON returned by the server into the format that Select2 wants. In the current version of our javascript, we say:

pagination: {
                      more: "true" //(params.page  < data.page_count)
                    }

It's funny, I actually just noticed that @dmeltzer had noticed this too and also found it curious - I think he was right :)

So we're actually using the JS to transform the correct result into an incorrectly-infinite-scrolling result.

I'll dig around in the git history to see why we decided to do that. But I suspect that's where the problem lies.

Another thing I was thinking - this is a little bit of added complexity that we could probably just remove entirely - e.g. we could get rid of the processResults callback entirely, and just returning correctly-formatted results from our /selectlist methods directly - since they exist solely for the purpose of backing our usage of Select2. We could probably do that all within our SelectlistTransformer too - and then there's less code to go through and everything gets a little easier to troubleshoot in the future.

@snipe
Copy link
Owner

snipe commented Jul 13, 2020

@uberbrady
make it so

@uberbrady
Copy link
Collaborator

There's a PR up against Master for these changes now - feel free to give it a try and let us know if it works for you.

@snipe
Copy link
Owner

snipe commented Jul 14, 2020

(The PR is located here: #8227 )

@snipe
Copy link
Owner

snipe commented Jul 14, 2020

I've merged that PR into master and it looks good - can anyone else confirm that this has resolved the issue?

@coaxke
Copy link

coaxke commented Jul 14, 2020

Just pulled master down as well - looks good to me too!

@snipe
Copy link
Owner

snipe commented Jul 14, 2020

Great - I'll close out this issue for now then.

Thanks again everyone for the great sleuthing, and thanks to @uberbrady for a quick fix.

rdj_thanks

@snipe snipe closed this as completed Jul 14, 2020
@snipe snipe added this to the Next Revision milestone Jul 14, 2020
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

No branches or pull requests

9 participants