always include foodcoop as parameter to a workerjob

[mortbauer]

• the way it was before failed jobs wouldn't run correctly anymore after the had failed because of the wrong scope
• now it is a bit more explicit but jobs can be re-run

[wvengen]

Ah, this seems to make sense.
Was this broken? If so, we may want to have this tested (but not for this PR).

As selecting the foodcoop scope is something used in various jobs, I would expect something like a before_action in the controller, but then for a job, or something similar. This action would be defined in the ApplicationJob.

I think the monkey-patching in the initializer is not the most beautiful approach, but it shows that we may want to have a generic way to select the foodcoop scope in a job. But perhaps this is hard to get right from an ApplicationJob or so.

I think if we'd resort to passing the foodcoop scope to perform_later, then we don't need the monkey-patch at all, right? Then perhaps a keyword scope argument may make sense, with the default nil for perform and the current scope for perform_later. Just some thoughts on making this a bit clearer.

[mortbauer]

I agree, actually I already found issues because activestorage jobs would then not run with the correct foodsoft selected, I think I found a better generic solution now.

[mortbauer]

I updated the pull request with a generic working solution.

[wvengen]

Ah, this is clearer. And as it this functionality is currently broken, I'd be willing to accept it as it is.

But I think there could be a cleaner solution that does not involve monkey-patching: can we make this a job concern, and include/extend it in each job that needs to select the foodcoop? It sounds to me like job callbacks could be used here instead, as a more standard mechanism. And perhaps a custom serializer to store the current scope. Let me know what you think.

[mortbauer]

yes a custom serializer could maybe work, but maybe also not because we already had serialize and deserialize overwritten which worked when normally scheduling the job, but somehow when retrying via resqueue it seemed to nether call serialize again but deserialize it would call again, resulting in an Exception obviously.
Making it a job concern is what I had in the 1 st version, but there are rails activestorage jobs which are also running and I think they might also need to run in the proper scope.

[wvengen]

Ah, that makes sense! Then this change seems good to me 👍

I see that the tests are failing, could you confirm that they pass locally, and rubocop too?

[mortbauer]

Can confirm that the test run, actually I created a pull request #37 where i slightly changed the dockerfile to use multistage building and use a target to run the tests, which I added as new workflow there.

[wvengen]

Thank you! 👍

[yksflip]

i think this works only for multi-coop mode and could be the reason why the tests fail with the default app_config.yml

[wvengen]

Ouch, I'm sorry not to have checked that!

[yksflip]

that's okay! Any idea how we can mitigate this?
just a wild guess:

      if FoodsoftConfig[:multi_coop_install]  do
        foodsoft_scope = @arguments.shift
        FoodsoftConfig.select_foodcoop foodsoft_scope
      end

[wvengen]

Or even only doing the monkey-patch for multi-coop installs, perhaps?
(with the downside that it's not tested, but perhaps it would be good to run CI tests with and without multicoop? that would be a different issue though)

[wvengen]

But perhaps it's not a good idea to read the config early on (it may involve database access, but didn't check).
The easiest solution may be

foodsoft_scope = @arguments.shift
FoodsoftConfig.select_foodcoop foodsoft_scope if FoodsoftConfig[:multi_coop_install]

We'd better shift also for singlecoop.

[yksflip]

alright, wasn't sure about what @arguments contains and if it needs to be shifted if called in a single-coop setting. I'll give this a try! :)
Having a pipeline testing multi-coop would be great, yes!

[mortbauer]

yes it only works for multicoop, but not only with my change, was only working with mulricoop since years