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

One attempt to fix the 'datatable double-request' problem. #10767

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

uberbrady
Copy link
Collaborator

We have a problem where a lot of bootstrapTables will AJAX request their data twice - once, that results in a 500. And once that actually works, and populates the table.

This is one example of fixing that for the Activity Report, which, if it works, we can take across to any other table that uses the bootstrap-table partial blade system.

It works by disabling the automatic enabling of the bootstrap table, waiting instead to allow the blade partial to do it instead.

This seems to work. Maybe? But we'll test before we get there.

@snipe
Copy link
Owner

snipe commented Mar 3, 2022

One thing - we reload that table on purpose, thought I don’t recall if that was for the check-all checkboxes, something in select2 or the popover tooltips, but we did definitely add that on purpose, so be careful how you go here and make sure you test all over the place where we use BS tables. It took extra effort to add that destroy and reload, so we wouldn’t have done it for no reason. (It was years ago though, so I’d have to look up why we did it.). Definitely make sure you test on more complex interfaces like the hardware listing, where we have advanced search, and an additional drop down menu, just to make sure it doesn’t break something we weren’t expecting.

@uberbrady uberbrady force-pushed the fix_double_request_datatable branch from f521f92 to 8d49d35 Compare March 8, 2022 19:47
@uberbrady uberbrady changed the base branch from master to develop March 8, 2022 19:47
@uberbrady uberbrady marked this pull request as ready for review March 8, 2022 19:49
@uberbrady uberbrady requested a review from snipe as a code owner March 8, 2022 19:49
@uberbrady
Copy link
Collaborator Author

I think this actually works, now?

🎶 I'm happy to settle any tests that you may think proper
I will test what I must test
To get this code away 🎵

@snipe
Copy link
Owner

snipe commented Mar 9, 2022

What does removing the data-toggle="table" do that benefits us? It seems to be important in the docs, no?

@uberbrady
Copy link
Collaborator Author

uberbrady commented Mar 9, 2022

That attribute triggers the automatic bootstrap-table-ing of any table with that attribute. But we don't need to do that, because we explicitly bootstrap-table-ify our own tables by setting their class to snipe-table, which we then bootstrap-table-ify in our own way here: https://github.com/snipe/snipe-it/pull/10767/files#diff-0fd6c93d0b4f746c594c9413b3c801f2541a062bb1f4865732979e7d9a483164L34

There were only two remaining instances of us leaving the data-toggle="table" on - once I removed them both, I think we no longer needed to destroy the table before recreating it? Happy to try and test it to make sure.

GitHub
We have a problem where a lot of bootstrapTables will AJAX request their data twice - once, that results in a 500. And once that actually works, and populates the table. This is one example of fixi...

@snipe snipe merged commit c817daa into snipe:develop Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants