Show "loading" screen for background reports and display when ready #10849

mkllnk · 2023-05-16T04:31:24Z

What? Why?

Closes Show generated report to user however long it takes #10752

Originally opened by @Matt-Yorkley against one of my pull requests:

Report Cable mkllnk/openfoodnetwork#1

What should we test?

Visit a reports page.
Compile a report and it should display as usual.
Activate background reports.
When you now compile a report, you see a loading screen until the report is ready. It then displays on screen or offers a download link.

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

mkllnk · 2023-05-16T05:00:47Z

@Matt-Yorkley Do you have time to work on this further? There are failing specs and URL generation fails:

ArgumentError: Cannot generate URL for orders_and_distributors_20230516.pdf using Disk service, please set ActiveStorage::Current.url_options.

drummer83 · 2023-05-16T14:29:17Z

Ah, nice! I wasn't aware of this draft!
It may close some of my ideas here: wishlist #448

mkllnk · 2023-05-24T06:06:55Z

Hm, need to work on a flaky spec here.

Matt-Yorkley · 2023-06-08T14:02:12Z

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 master without these changes, so it makes it somewhat hard to gauge whether this is really adding to the flakyness or not.

I've used the Sidekiq::Testing.inline! helper with a block around submitting the reports form(s) after noticing @filipefurtad0 mentioning it. It seems like it helps

mkllnk · 2023-06-08T23:26:01Z

I've used the Sidekiq::Testing.inline! helper with a block around submitting the reports form(s) after noticing @filipefurtad0 mentioning it. It seems like it helps

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.

mkllnk · 2023-06-09T01:31:32Z

I can reproduce the flaky spec on the previous commit:

$ ./script/rspec-slow-repeat 10 spec/system/admin/reports_spec.rb:40
Running 10 times with 4 cores
    !!! Fail !!!
    !!! Fail !!!
Pass.
Pass.
    !!! Fail !!!
Pass.
    !!! Fail !!!
Pass.
    !!! Fail !!!
    !!! Fail !!!
4 of 10 passed (40%)
Exiting...
Check tmp/rspec.log for details.

The latest commit with inline processing doesn't help either:

$ ./script/rspec-slow-repeat 10 spec/system/admin/reports_spec.rb:40
Running 10 times with 4 cores
    !!! Fail !!!
    !!! Fail !!!
/home/maikel/code/openfoodnetwork/config/application.rb:51: warning: already initialized constant Spree::Config
/home/maikel/code/openfoodnetwork/config/application.rb:51: warning: previous definition of Config was here
/home/maikel/code/openfoodnetwork/config/initializers/content_config.rb:2: warning: already initialized constant ContentConfig
/home/maikel/code/openfoodnetwork/config/initializers/content_config.rb:2: warning: previous definition of ContentConfig was here
2023-06-09 11:32:40 +1000 Rack app ("GET /cable" - (127.0.0.1)): #<TypeError: superclass mismatch for class TestCard>
    !!! Fail !!!
/home/maikel/code/openfoodnetwork/config/application.rb:51: warning: already initialized constant Spree::Config
/home/maikel/code/openfoodnetwork/config/application.rb:51: warning: previous definition of Config was here
/home/maikel/code/openfoodnetwork/config/initializers/content_config.rb:2: warning: already initialized constant ContentConfig
/home/maikel/code/openfoodnetwork/config/initializers/content_config.rb:2: warning: previous definition of ContentConfig was here
/home/maikel/code/openfoodnetwork/config/application.rb:51: warning: already initialized constant Spree::Config
/home/maikel/code/openfoodnetwork/config/application.rb:51: warning: previous definition of Config was here
/home/maikel/code/openfoodnetwork/config/initializers/content_config.rb:2: warning: already initialized constant ContentConfig
/home/maikel/code/openfoodnetwork/config/initializers/content_config.rb:2: warning: previous definition of ContentConfig was here
Pass.
Pass.
Pass.
    !!! Fail !!!
    !!! Fail !!!
    !!! Fail !!!
    !!! Fail !!!
3 of 10 passed (30%)
Exiting...
Check tmp/rspec.log for details.

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 .loading div is present. PR update is coming.

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.

mkllnk · 2023-06-09T06:21:51Z

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.

drummer83 · 2023-06-15T19:24:08Z

Hi @mkllnk,
Thanks for the continuing work on this!

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.
With background reports enabled I got an (kind of) endless spinner. No notification about an email or anything. I never got to see a link, an email or the report.

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?

mkllnk · 2023-06-15T23:18:04Z

I tried the Order Cycle Supplier Totals report as super admin, so with a loooot of data.

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.

mkllnk · 2023-06-15T23:18:55Z

Your UX suggestions are great. But I think that they should be handled in a separate pull request.

drummer83 · 2023-06-21T13:18:36Z

Hi @mkllnk,
Thanks for the hint regarding fresh deployment and consumed memory. I will take that into account. 👍

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).

drummer83 · 2023-06-25T16:19:56Z

Ok, so after a fresh deploy to staging UK we have the following situation with background reports ON:

Report generation duration	On screen	Send email notification
t < 5 seconds	display report ✔️	no ✔️
5 seconds < t < 2 minutes	when report ready: display report ✔️	yes ✔️
t > 2 minutes	after 2 min: display email hint (this may fail on slow servers, ignore) ❌	not yet ✔️
	when report ready: display report ✔️	yes ✔️

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

In principle the reports are working.
Reports taking longer than 2 minutes will be displayed as well (when ready) ✔️
The email hint never showed on screen. ❌
I may have managed to 'overload' the system (or a worker) with too large data amount - I hope there is a mechanism to recover it automatically. ❓
Usability still has room for improvement. 😉 wishlist #448

I will move to 'Ready To Go' but I'm leaving it for you @mkllnk to merge.
Thanks!

mkllnk · 2023-06-26T00:25:44Z

The email hint never showed on screen.

That's fine. Your wishlist has better ideas for this.

I may have managed to 'overload' the system (or a worker) with too large data amount

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.

Matt-Yorkley · 2023-06-28T11:52:09Z

I may have managed to 'overload' the system (or a worker) with too large data amount - I hope there is a mechanism to recover it automatically.

Can you give a bit more detail on this, @drummer83 ? What happened?

drummer83 · 2023-06-28T21:56:46Z

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.

This is what I experienced as super admin user on staging UK. The email never arrived. @Matt-Yorkley

mkllnk self-assigned this May 16, 2023

mkllnk changed the title ~~Report cable~~ Show "loading" screen for background reports and display when ready May 16, 2023

mkllnk mentioned this pull request May 16, 2023

Report Cable mkllnk/openfoodnetwork#1

Closed

Matt-Yorkley force-pushed the report-cable branch 2 times, most recently from 5b21a40 to e348a43 Compare May 16, 2023 12:31

Matt-Yorkley force-pushed the report-cable branch from fb5d844 to fe5eca4 Compare May 19, 2023 10:32

Matt-Yorkley self-assigned this May 23, 2023

Matt-Yorkley marked this pull request as ready for review May 23, 2023 14:07

mkllnk force-pushed the report-cable branch from fe5eca4 to 6be734c Compare May 24, 2023 04:03

Matt-Yorkley force-pushed the report-cable branch 2 times, most recently from e4caae4 to 50dc375 Compare June 1, 2023 19:03

Matt-Yorkley and others added 9 commits June 9, 2023 14:44

DRY admin javascript imports

242c74c

Load reports show page without all the legacy javascript

ced959a

Run reports in background

a3ef604

Update report email spec

b230b37

Improve flakyness on report specs

7caf4b0

Remove outdated report timeout handling

e56c065

Since we don't wait for the report any more, a timeout is very unlikely and we don't need special handling for it.

Style Layout/MultilineMethodCallIndentation

d312a59

mkllnk force-pushed the report-cable branch 2 times, most recently from 2deb5da to 20535dd Compare June 9, 2023 06:07

drummer83 self-assigned this Jun 15, 2023

drummer83 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jun 15, 2023

drummer83 removed their assignment Jun 15, 2023

mkllnk removed the feedback-needed label Jun 15, 2023

drummer83 self-assigned this Jun 21, 2023

drummer83 added pr-staged-au staging.openfoodnetwork.org.au pr-staged-fr staging.coopcircuits.fr and removed pr-staged-au staging.openfoodnetwork.org.au labels Jun 21, 2023

drummer83 added blocked and removed pr-staged-fr staging.coopcircuits.fr labels Jun 21, 2023

drummer83 removed their assignment Jun 21, 2023

sigmundpetersen removed the blocked label Jun 22, 2023

drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 22, 2023

drummer83 self-assigned this Jun 22, 2023

drummer83 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jun 23, 2023

drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 25, 2023

mkllnk merged commit a7915b8 into openfoodfoundation:master Jun 26, 2023

mkllnk deleted the report-cable branch June 26, 2023 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show "loading" screen for background reports and display when ready #10849

Show "loading" screen for background reports and display when ready #10849

mkllnk commented May 16, 2023 •

edited by Matt-Yorkley

Loading

mkllnk commented May 16, 2023

drummer83 commented May 16, 2023

mkllnk commented May 24, 2023

Matt-Yorkley commented Jun 8, 2023

mkllnk commented Jun 8, 2023

mkllnk commented Jun 9, 2023 •

edited

Loading

mkllnk commented Jun 9, 2023

drummer83 commented Jun 15, 2023

mkllnk commented Jun 15, 2023

mkllnk commented Jun 15, 2023

drummer83 commented Jun 21, 2023

drummer83 commented Jun 25, 2023

mkllnk commented Jun 26, 2023

Matt-Yorkley commented Jun 28, 2023

drummer83 commented Jun 28, 2023

Show "loading" screen for background reports and display when ready #10849

Show "loading" screen for background reports and display when ready #10849

Conversation

mkllnk commented May 16, 2023 • edited by Matt-Yorkley Loading

What? Why?

What should we test?

Release notes

mkllnk commented May 16, 2023

drummer83 commented May 16, 2023

mkllnk commented May 24, 2023

Matt-Yorkley commented Jun 8, 2023

mkllnk commented Jun 8, 2023

mkllnk commented Jun 9, 2023 • edited Loading

mkllnk commented Jun 9, 2023

drummer83 commented Jun 15, 2023

mkllnk commented Jun 15, 2023

mkllnk commented Jun 15, 2023

drummer83 commented Jun 21, 2023

drummer83 commented Jun 25, 2023

Summary

mkllnk commented Jun 26, 2023

Matt-Yorkley commented Jun 28, 2023

drummer83 commented Jun 28, 2023

mkllnk commented May 16, 2023 •

edited by Matt-Yorkley

Loading

mkllnk commented Jun 9, 2023 •

edited

Loading