-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Show "loading" screen for background reports and display when ready #10849
Conversation
@Matt-Yorkley Do you have time to work on this further? There are failing specs and URL generation fails:
|
5b21a40
to
e348a43
Compare
Ah, nice! I wasn't aware of this draft! |
fb5d844
to
fe5eca4
Compare
Hm, need to work on a flaky spec here. |
e4caae4
to
50dc375
Compare
Hey @mkllnk I've added a couple of things here to try to deal with the flaky spec issues. I've noticed over the last week that we already have flaky specs across multiple reports-related tests in I've used the |
It means that the report is generated straight away and ready before the controller action ends. So we are not testing the path of a report taking longer... I'll have a look. |
I can reproduce the flaky spec on the previous commit:
The latest commit with inline processing doesn't help either:
I found a way to avoid the flakiness. We have to wait for the loading spinner to appear and disappear. You can't see it in specs but the |
This is not a normal pattern for setting up ActionCable channels, so it might need some notes. It ensures the broadcasts from the ReportJob are unique not just to the user session but also to the specific tab in the user's browser. Otherwise if the user has two different report pages open in separate tabs with the same session, the broadcast would overwrite the #report-table element in both of them.
Since we don't wait for the report any more, a timeout is very unlikely and we don't need special handling for it.
Rubocop was complaining about too many arguments. But `ApplicationJob#perform` needs all arguments handled in one call. While we could allow the `perform` method generally to have more arguments, there could be other methods called `perform` which should still be scrutinised. Instead, it seems acceptable to me to have more arguments as long as they are clearly named as keyword arguments. Rails uses this a lot to document all options including their default values, for example in Active Storage. It's better then bundling several arguments in an undocumented hash just to reduce the number of given arguments. And once we upgraded to Ruby 3.1, we can clean the method calls up as well. `call(user: user)` becomes `call(user:)` without repetition.
2deb5da
to
20535dd
Compare
This spec seems to be stable now but I discovered a race condition. I assume it to be rare in production but a busy server could spark this more often. I've seen people delaying job executions by a second or so to avoid this but that's bad UX if people always have to wait a second to see the report. There must be a better way. Maybe we can show the loading screen on form submission within the browser instead of via cable car. |
I'm hiding a real bug here. There's a race condition when the cable event of the finished report is sent before the loading spinner rendered. The spinner then overwrites the report again. I added a spec for that but don't have a solution yet. I also noticed that the loading spinner is not displayed in testing but we can assert on the CSS class of the container.
Hi @mkllnk, I tried the Order Cycle Supplier Totals report as super admin, so with a loooot of data. With background reports disabled I got an error 500 (timeout) after 120 s. While we are working on this, what do you think about this proposal: wishlist #448? I think this would be good usability. What do you think? I don't see an improvement here, sorry... Am I missing something? |
You may have used a lot of memory then. When you switch to background reports, it's good to also do a deploy or restart puma so that the memory is freed up again for background reports to use them. Did you also try a smaller report first? It would be good to know if it works in principle. It did in the past but I'm wondering if any recent merges to master broke something, like the Redis errors we are seeing. I looked at the log files and the Sidekiq metrics and it seems that 4 background reports were successfully run, taking an average of 2 minutes each. |
Your UX suggestions are great. But I think that they should be handled in a separate pull request. |
Hi @mkllnk, Ok, I tried again, this time on staging AU and FR, but we don't have enough data here to run into the timeout (120 s). So this one is blocked until I can use staging UK again (after merging #10939). |
Ok, so after a fresh deploy to staging UK we have the following situation with background reports ON:
I used the reports 'Orders and Distributors' and 'Order Cycle Supplier Totals' and increased the date range. The 'Orders and Distributors' report with no date limitation at all ended up in an 'endless' spinner and no results were displayed, no email was sent - that is after 20 minutes or so. I will update if it arrives eventually. Summary
I will move to 'Ready To Go' but I'm leaving it for you @mkllnk to merge. |
That's fine. Your wishlist has better ideas for this.
That's still possible but less likely. If this becomes a problem in production, which has a lot more memory, then we may need to invest more work into this. There are ideas already. |
Can you give a bit more detail on this, @drummer83 ? What happened? |
This is what I experienced as super admin user on staging UK. The email never arrived. @Matt-Yorkley |
What? Why?
Originally opened by @Matt-Yorkley against one of my pull requests:
What should we test?
Release notes
Changelog Category: User facing changes
The title of the pull request will be included in the release notes.