It looks like we have a new race condition that may only be a problem in
specs. If you trigger one report, it displays via websockets and then
you trigger the next report, there may still be some Javascript active
that displays the first report while the second one is loading. I'm not
sure if users would navigate that fast though.
To minimise the problem, I adjusted the polling to leave more room for
the default websockets response.
This also resolves a race condition scenario. Even if the report gets
rendered via websockets before the controller response is rendered then
the fallback script loads the report again. It's not the most beautiful
but probably okay until we replace websockts altogether.
I'm leaving websockets in at the moment because it can render the report
much quicker than polling can.
I'm adding TurboPower for the scroll_into_view action. It adds all the
nice CableReady actions to Turbo Streams.
Note that I omitted `block: "start"` because that option is the default
in Javascript. And the generic `action` method doesn't support
parameters like this anyway. I'll work on that in the next commit.
I also re-introduced a race condition by rendering the "loading"
indicator after triggering the report rendering job. I'm planning to
resolve that later.
Sending large reports via Cable Ready is unreliable. The events are
dropped at an unknown point and the report is never displayed to the
user. Instead we just send a link to the report via Cable Ready and
offer a button to load the report on screen.
This has the UX benefit of warning the user about the size as well.
Weaker devices can struggle rendering big HTML documents.
We had a race condition that could first display the report and then
replace it again with the "loading" spinner. That doesn't seem to happen
now that we changed the order of cable events.
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.
It was bad UX to show a link that doesn't work straight away. At the
time, it was the only way to download the report but now we send an
email which is a much better way to go.
I leave the rest of the code because we want to implement a reflex which
shows the download link once it's ready.
I added a system spec to verify that the download link can be generated
within the mailer in a background job. ActiveStorage is a bit particular
when it comes to genererating URLs and depending on the situation it may
generate a redirect URL, a proxy URL or link directly to the storage.
But we want a redirect URL here.
The `login_as_admin_and_visit` helper was used a lot but isn't really
shorter than:
login_as_admin
visit path_visit
Calling those methods separately reduces line length. It also removes
the potential impression that it may be more efficient to use the
helper. Now we have less indirection if one of the calls fails and see
the failing spec line straight away.
It was often used with a `visit` statement after which resulted in
unnecessary page loads. There was only one case where a `visit` was
expected but it wasn't needed either.
