Illustrate a different pattern for lv_state

[davefrey]

This PR is to show and get feedback on a proposed common way to manage the LV state needed during a request. It only shows the pattern for one controller and still needs to be applied to other controllers/commands/views.

Managing liquidvoting state in controllers and views is a little messy, and often (but inconsistently) uses a bare @lv_state.

The proposed usage pattern: controllers deliberately refresh_lv_state at the appropriate times (whenever that state will have changed, from votes or delegations), and then expose this state with a lv_state view helper. The @lv_state attribute would no longer be directly referenced.

So, thoughts about this being the standard idiom? Can anyone improve on the lv_state and refresh_lv_state names? I think it's best to make these things explicitly liquid voting, so that it's clear in views and controllers what is LV-related.

[oliverbarnes]

I like the concept of a refresh. It got me wondering whether we shouldn't be doing this in the same requests as the mutations, though, and save this extra request. Graphql is meant for precisely for this sort of thing - queries tailored for the exact needs of the frontend, without a lot of changes to the api.

But does muddle the waters again regarding where this request should live (whether module or api client)

as far as naming, maybe refresh_from_api and api_state? This naming is internal to our module, so it doesn't need to be unique across decidim itself (other modules can safely use the same naming)

[davefrey]

Thanks and I like your api names better, I'll change that.

For state requests: controllers (and commands) indeed need a refresh after any api mutation, so we could return the info in the mutator response. We'd still need a way to ask just for the state though (eg when ProposalsController is going to first render the support/delegation UI).

And I thought about if we should put the refresh call in the Liquidvoting module, but that's then a two-way coupling with the controller, and the module is setting state back on the controller. We can also move that refresh call into a LiquidvotingHelper, where view helpers would normally go, but then we have yet another not-inline place for LV stuff. Having each "client" (any controller that will render our UI) carry a private attribute exposed as a helper + a private method to refresh seems the most transparent to me. WDYT?

So "where" is one open question, the second is "do we retrieve api state both in the mutator response + a helper for those other cases, or just the helper for all the cases"?

Edit: for the second question, it's more efficient in mutator cases to return the data in the response, rather than systematically doing refresh_from_api ... but then the usage pattern gets messier. So always doing two calls looks like

Liquidvoting.create_delegation(delegator_email, params[:delegate_email], proposal)
refresh_lv_state

whereas leveraging the mutator response turns into something like

response = Liquidvoting.create_delegation(delegator_email, params[:delegate_email], proposal)
refresh_lv_state(response)

[...]

def refresh_lv_state(api_response = nil)
  if api_response
    @lv_state = (dig the data out of the api response, or ask `Liquidvoting` to transform like `user_proposal_state` below)
  else
    @lv_state = Liquidvoting.user_proposal_state(delegator_email, proposal_locator.url)end
  end
end

We can pair this if you like, we still have the first question of where to put this stuff

[oliverbarnes]

For state requests: controllers (and commands) indeed need a refresh after any api mutation, so we could return the info in the mutator response. We'd still need a way to ask just for the state though (eg when ProposalsController is going to first render the support/delegation UI).

Good point, I had forgotten about the initial state request.

whereas leveraging the mutator response turns into something like

I like this 👍

we still have the first question of where to put this stuff

I wouldn't change where things are at, the module is in itself a helper bucket? not sure I follow the bit about two-way coupling

[davefrey]

Good, seems like this is firming up, I'll push changes tomorrow.

By two-way coupling, I meant the Controller asking the Module to update api_state on the Controller, which would be weird, but is not needed. The version of def refresh_lv_state(api_response = nil) above seems the right solution w/o weird coupling.

I think we're aligned, if not lmk on the next push.

[oliverbarnes]

I see it as the controller simply calling a method in its enclosing module, which encapsulates all helpers 🤷

[davefrey]

I'll undraft this, it goes a long way towards cleaning up how we use api_state.

(1) What's missing is moving from this:

Liquidvoting.create_delegation(delegator_email, params[:delegate_email], proposal)
refresh_from_api

to this (which leverages the api response to capture api_state, rather than making a second api call):

response = Liquidvoting.create_delegation(delegator_email, params[:delegate_email], proposal)
refresh_from_api(response)

I have local work to do this refresh_from_api(response) but it became messy and I didn't want to block the PR on resolving. It involves extending the graphql queries and adding a transform to build the same UserProposalState struct as Liquidvoting.user_proposal_state; it feels like there's a better approach and I didn't want to force it in now.

In consequence, we have a clean, correct, inefficient (because 2nd api call) pattern for dealing with api_state; see the first snippet above.

(2) The other potential improvement on the current PR is to extract @api_state and def refresh_from_api from the three controllers into a helper, but that needs a bit of adapting + it puts LV logic in a new helper file. We might also extract a concern.

I propose we do these things (1) and (2) separately, if at all.