Decouple decidim api client from the decidim lv_state object

[davefrey]

In client.rb we have both api client code, and support for retrieving a current LV state object.

Since we want to align with ruby-client, which is independent of any integration, we need to separate these two things. Also, we will eventually replace client.rb with a gem.

One approach might be to have non-client.rb code pass a graphql query string to a generic_query method inside client.rb, and wrap the result in a state struct.

[oliverbarnes]

Hmm, I see what you mean about needing a custom query... but I think preparing a graphql query on decidim's side also defeats the purpose of using the ruby client.

We can pair on this when the time comes, to try and find a way to encapsulate that in the client while building the decidim-specific state in decidim

[davefrey]

Yes, I don't like the custom query too much. The idea was to leave the four mutator queries, that's the api for any LV client using ruby-client gem. The decidim code would use those four, plus fall back to passing graphql string for the current state, but right now it seems we should be able to provide a current state query and supporting ruby struct in a generic, non-decidim way.

Pairing is perfect.

[davefrey]

I started a discussion #66 for how to do this.

[oliverbarnes]

Hey guys, to expedite things, I'm settling on the solution proposed on #66 (comment):

# lib/decidim/liquidvoting.rb
module Decidim
  module Liquidvoting
    
    # previously part of the api client
    def user_proposal_state(user_email, proposal_url)
      # Talks to the api through the client
    end

    # previously on Decidim's Proposal model
    def update_vote_count(proposal)
      # Saves to the db using activerecord's update_column() 
    end
  end
end

We can iterate on this later if we aren't happy with it after the change.

[davefrey]

Summarizing from a call just now with Oliver:

We agree to the three boundaries he outlines in the conversation:

1. an "integration-neutral" client.rb (to be api-client gem)
2. a Decidim-LV space, roughly a facade that can take form as helper methods in the Decidim::Liquidvoting module
3. "pure" Decidim code and our overrides in decidim-module-liquidvoting

We can make client.rb more neutral by extracting the current_proposal_state method and ProposalState structure into our module helpers. To do this we'll expose some lower-level fetch methods in client.rb and have our module helpers do the convenience bundling. This still lets us do the optimisation that is needed in client.rb, and doesn't prevent us from moving some convenience bundling back into client.rblater when we have more information from a post-Decidim integration.

We agreed it's not worth spending a lot of time here, so if this extraction gets messy we'll talk again.

[davefrey]

Adding a Decidim::Liquidvoting.update_vote_count(proposal) helper raises a question: should we just back out our override of Proposal, since we can update_columns(proposal_votes_count: new_lv_count)?

I think we should 1) move the update call into the helper, and 2) still override the original Proposal#update_votes_count method to be a no-op, and maybe still log it. If we leave the method in place we risk other Decidim code stepping on our vote count.

[oliverbarnes]

That's a good question. But in the end we can already see that in the logs, if it gets called (the sql query is already logged), and locking our module to a given decidim version ensures no new surprise decidim code will be added.

The cleanest and less defensive thing is to ensure all calls to Proposal#update_votes_count are replaced, and then manually test and watch logs for surprises.

This also reminds me we should instrument both our decidim instance and our module to post events and logs to Honeycomb. I'll add an issue for that

[davefrey]

Proposal#update_votes_count looks to be very contained ✅

However, it's wrapped in an :after_(save|destroy) callback, and those are a little harder to track. Anything that creates/deletes ProposalVote objects, including via the Proposal.votes association, will fire the callback and step on the LV vote count. rake db:seed is one way this happens ... maybe there are others?

For me, it seems high risk to lose the LV count until the next api mutation results in it being corrected. The safest way would be to make the method a no-op, but lmk what you think.

I think it also better communicates to anyone looking at the code that LV has taken ownership of that attribute.

Edit: the proposal I'm making is to 1) override Proposal#update_votes_count to be a no-op, and 2) do the actual updating from our module update_proposal_votes_count helper (same approach with a update_proposal_endorsements_count)

[oliverbarnes]

Hm, the callback context does complicate things.

Adding the no-op is still unsafe, imo, because if it is called, it means we've missed a scenario where we should be calling LV instead, and the count will be inconsistent because of that. And it defeats the purpose of neatly isolating the change: we'd still be changing the proposal model, and adding a new way of doing things at the same time, adding complexity. If we're changing the model, we might as well leave the change there, covering for unforeseen scenarios that the callback covers.

There's another issue we need to double check, as well - is the proposal model used in other places besides supports and endorsements, in, say, initiatives or assemblies?

One way to map out where Proposal#update_votes_count is called is to log it with information of where it's being called from, then run the test suite? We might need to do this regardless of the refactoring, to be sure our override doesn't break Decidim in other places we're not deliberately overriding.

[davefrey]

I'm thinking how hard it's going to be to debug if we have unexpected Proposal#update_votes_count calls, so I agree we need to do what we can to find them. Running a full regression is the best we have for coverage.

One approach is to 1) keep logging, 2) run the full spec suite, 3) see what shows in the log. I think this can work if we keep a discipline to redo this exercise each time we bump decidim gems. Let's call this a "decidim version verification" process, a list of things to do when we bump gems.

My first instinct was to put a fail "Unexpected call to Proposal#update_votes_count" in that method to cover cases where we forget, raising gives us a stacktrace too, and then override or whitelist the unexpected calls: raise catches everything always, though this is pretty user-unfriendly for things we miss. I'm not completely trusting verification, and am afraid of vote count bugs.

Maybe we can log specifically enough to surface the event with our instrumentation: when it happens, we're alerted. That might be a

[oliverbarnes]

failing / raising works to trace this in the test suite 👍

I didn't quite get what you meant about not trusting verification 😄 And your last sentece was cut off

I think we should:

1 - use fail / raise locally to roughly trace where the method gets called
2 - add a nice visible log on the method for any other calls we don't detect for now, and commit this
3 - finish the refactor

Feel free to make the method noop if it makes you more comfortable, but in the end it'll give us inconsistent results anyway. So the only way forward is to detect where it's still called progressively, and replace the call with LV calls. With time we should have this covered for a given Decidim version.

[davefrey]

Ah, for not trusting: I meant inspecting to find callers, and remembering to do it again when bumping Decidim gems. It felt too informal for what would be a difficult bug to find.

I no longer like the no-op approach, you're right it's the same problem in either case...let's do a best-effort (via fail + full Decidim specs run), and leave the logging in the method, but otherwise leave it operational.

I'm finishing the refactor today, and I'll open a separate issue for doing and documenting this best-effort to find Decidim callers of Proposal#update_votes_count.

[oliverbarnes]

Ah, for not trusting: I meant inspecting to find callers, and remembering to do it again when bumping Decidim gems. It felt too informal for what would be a difficult bug to find.

I think the best we can do is incrementally detect the calls over time, and we'll eventually have a good grasp.

I expect upgrading will also be easier than the first time around.

I no longer like the no-op approach, you're right it's the same problem in either case...let's do a best-effort (via fail + full Decidim specs run), and leave the logging in the method, but otherwise leave it operational.

ok :)

I'm finishing the refactor today, and I'll open a separate issue for doing and documenting this best-effort to find Decidim callers of Proposal#update_votes_count.

Makes sense, good call 👍