Add Stimulus for user feedback showing FieldSlip PDF job status

nimmolo commented 3 months ago

New Stimulus controller to give on-page feedback on the progress of generating PDFs.

For returning data from the FieldSlipJobTracker, this adds a new AJAX endpoint: a tiny FieldSlipJobTrackersController with just a show action. It returns the JSON for time, pages, and status.

New intended workflow

User creates a PDF with x number of pages
The Projects::FieldSlipsController :create action responds with a turbo_stream that prepends a new row (because they're most recent first) to the table of field slip PDF jobs, instead of redirecting to the Project :show page
The new row is "noticed" by the Stimulus controller, which starts a timer to check for updates
Stimulus checks the new FieldSlipJobTrackersController :show endpoint for the current status of the tracker every second
The :show endpoint responds with a turbo_stream that updates the row, until the status is done

nimmolo commented 3 months ago

@mo-nathan — If you can add the "pages" data to the tracker, i'll add it to the controller here.

nimmolo commented 3 months ago

New approach - use Turbo!

This use case is actually a good illustration of how Stimulus can be simpler with Turbo, and how it requires a simpler way of thinking about JS, which is obviously not yet second nature for me, either.

Instead of writing the Stimulus controller to explicitly update the targets in each row, our FieldSlipJobTrackersController can just return a turbo_stream response with the refreshed HTML for the entire row (identified by DOM id), and Turbo will update the row without us handling all the HTML in JS.

coveralls commented 3 months ago

coverage: 94.391% (+0.002%) from 94.389% when pulling deb54c8aa99b7017ea53808d75fa954df92300ac on nimmo-field-slip-stimulus into e4cce493cbe27b6f36a348ed7c73699b332408ea on njw-field-slip-pdfs.

nimmolo commented 3 months ago

Workflow description updated above.

Current issue: it's not finishing the jobs.

I had mistakenly thought they were done, but the done method is not a property, it declares them done and saves the job.

JoeCohen commented 3 months ago

@nimolo

If you have a recent db snapshot, you can use /projects/341 or /projects/334 (FWIW, I've been using the uncleaned recent snapshots, rather than the Oct. clean snapshot. The disadvantage is that you can't login as someone else. My workaround is a second account that uses a different email.)
Else just create a new project (including a Field Slip Prefix)

nimmolo commented 3 months ago

Polling the endpoint is making a LOT of requests for me locally because my jobs aren't getting marked as "done", and although this is a development bug, I figure in the case of any glitch, we don't want to tie up someone's browser with zombie requests like this.

I believe a better way to do this may be, rather than polling the endpoint, to make the FieldSlipJobTracker model broadcastable. Turbo broadcasts are something I wanted to try anyway, and they were created for exactly this situation.

mo-nathan commented 3 months ago

The new row isn't showing up until I refresh the page. And I'm seeing bunch of tiny lines with text show up at the bottom of the page.

nimmolo commented 3 months ago

Yes - I missed an ID change. Updated!

mo-nathan commented 3 months ago

That gets the row in, but it still doesn't update every second. I just ran a batch of 30 and it showed the initial row, but then sat there without doing any updates. When I eventually (> 10 seconds) refreshed the page, it reported that it is Done and took a total 5.9 seconds.

nimmolo commented 3 months ago

Yeah, that's what's confusing me too. I thought maybe the tracker instances were not getting freshly retrieved, so I tried reloading the records in the controller, and that didn't work either, so then i debugged and checked the records directly in the console.

Whenever I did that, they hadn't budged past "Starting".

I believe if you check your browser developer tools console, you'll also find that it just keeps polling the endpoint.

mo-nathan commented 3 months ago

If they aren't getting past "Starting" in the database, that means SolidQueue is not running. Remember you have to restart the SolidQueue process if you change code at all even in a completely unrelated part of the system. I'm not sure why that makes it not work, but it's very different from working with the rails server.

nimmolo commented 3 months ago

Ahaa. Interesting. Thanks!

I wonder if Solid Queue can work with the foreman gem. Ok - yes it does.

Foreman is a gem that runs a script with a proc to start and stop processes and compile assets. It is the default way to run Rails dev servers currently. The reason I didn't switch to it yet was because it requires us to use a different command to start the dev server: /bin/dev instead of rails s. I didn't want to confuse people.

But check that link out when you get a chance, (you too @JoeCohen and @pellaea) and see if you think switching to foreman is a tolerable option to simplify running the dev server. It's an unanswered question though, if it will restart solid_queue on code changes. I'm not finding where it does that.

A small bonus is that it should clear up our scss deprecation warnings on CI.

JoeCohen commented 3 months ago

foreman is above my pay grade. I didn't understand anything. I do worry about moving away from the Rails main stream.

On Thu, Apr 18, 2024 at 2:51 PM andrew nimmo @.***> wrote:

Ahaa. Interesting. Thanks!

I wonder if Solid Queue can work with the foreman https://github.com/ddollar/foreman gem. Ok - yes it does https://www.bigbinary.com/blog/migrating-to-solid-queue-from-sidekiq#3-starting-solid-queue .

Foreman is a gem that runs a script with a proc to start and stop processes and compile assets. It is the default way to run Rails dev servers currently. The reason I didn't switch to it yet was because it requires us to use a different command to start the dev server: /bin/dev instead of rails s. I didn't want to confuse people.

But check that link out when you get a chance, (you too @JoeCohen https://github.com/JoeCohen and @pellaea https://github.com/pellaea) and see if you think switching to foreman is a tolerable option to simplify running the dev server.

A small bonus is that it should clear up our scss deprecation warnings on CI.

— Reply to this email directly, view it on GitHub https://github.com/MushroomObserver/mushroom-observer/pull/2104#issuecomment-2065384305, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALDFFRFBFOVWUCA3MNK5DY6A56NAVCNFSM6AAAAABGL3ENASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRVGM4DIMZQGU . You are receiving this because you were mentioned.Message ID: @.***>

nimmolo commented 3 months ago

Whew! Debugged the stimulus. It now works as advertised.

It turns out it works with async, without making too many requests. The "too many" part came from my bad syntax with setInterval - we should only pass the function name, not the executed function results.

// it's the difference between 
setInterval(this.do_thing, 1000)
// and 
setInterval(this.do_thing(), 1000)
// but note `this` cannot work anyway! see below

The other trouble was that we can't call a class function like this.do_thing within setInterval, because we lose the context of this. It may be possible within the IIFE as I've redone it here, but there's no need. Our function is very short, so I inlined it.

  // Every second, send a get request to find out the status of the PDF.
  // NOTE: Can't call a class function from `setInterval` because it resets
  // the context of `this`
  start_timer_sending_requests() {
    if (this.status_id != "3") {
      // Set the intervalId to the interval so we can clear it later
      this.intervalId = setInterval(async () => {
        // console.log("sending fetch request to " + this.endpoint_url)
        const response = await get(this.endpoint_url,
          { responseKind: "turbo-stream" });
        if (response.ok) {
          // turbo-stream prints the row in the page already
        } else {
          console.log(`got a ${response.status}`);
        }
      }, 1000);
    } else if (this.intervalId != null) {
      clearInterval(this.intervalId)
    }
  }

Figured out working syntax for updating that row partial, in the controller response block.

# app/controllers/field_slip_job_trackers_controller.rb

  def show
    return unless (@tracker = FieldSlipJobTracker.find(params[:id]))

    respond_to do |format|
      format.turbo_stream do
        render(turbo_stream: turbo_stream.replace(
          :"field_slip_job_tracker_#{@tracker.id}", # id of div to replace
          partial: "projects/field_slips/tracker_row",
          locals: { tracker: @tracker }
        ))
      end
    end
  end

nimmolo commented 3 months ago

I think all this needs now is pages for the tracker.

mo-nathan commented 3 months ago

Sounds great! Thanks! I’ll check it out in the morning and add the pages bit.

On Thu, Apr 18, 2024 at 10:03 PM andrew nimmo @.***> wrote:

I think all this needs now is pages for the tracker.

— Reply to this email directly, view it on GitHub https://github.com/MushroomObserver/mushroom-observer/pull/2104#issuecomment-2065615293, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYQDURAUDYZUV72F6EQG6DY6B3NPAVCNFSM6AAAAABGL3ENASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRVGYYTKMRZGM . You are receiving this because you were mentioned.Message ID: @.***>

nimmolo commented 3 months ago

The remaining errors seem to relate to the changes in how the create action works now.

I'll wait for the pages attribute before trying to sort it out.

MushroomObserver / mushroom-observer

Add Stimulus for user feedback showing FieldSlip PDF job status #2104