search

liquidvotingio / decidim-module-liquidvoting

GNU Affero General Public License v3.0
4 stars 0 forks source link

Hash out tasks for endorsements integration #31

Closed oliverbarnes closed 3 years ago

oliverbarnes commented 3 years ago

Update by @jinjagit

EDIT: - plan revised after discussions below. Plasn is now:

oliverbarnes commented 3 years ago

this can easily become an umbrella issue as well. Once we have individual tasks, let's toss it

jinjagit commented 3 years ago

I've added a list of steps in 1st comment. I'll convert these to individual issues.

jinjagit commented 3 years ago

Probably better if I ask for comments / thoughts before converting to individual issues. Any suggestions / comments?

davefrey commented 3 years ago

It's pretty good, here's what I noticed:

To implement "either supports or endorsements, at the Proposal level", you'll need database support, so add a migration for proposals.delegation_subject (:support, :endorsement), as well as the rake task for copying local migrations. Also, this should reuse the same delegations controller, but rename ProposalVoteDelegationsController to ProposalDelegationsController. Also create a helper for the views to ask a helpful question like if delegating_supports|endorsements.

davefrey commented 3 years ago

I think it's worth copying/adapting the specs for endorsements, and using a red spec to drive the implementation...it's pretty good for focus since it's implementing outside-in, and when it's green you're done with happy path.

davefrey commented 3 years ago

I like your leaving the admin UI for the end. You can also defer the database work, and just hardcode a value that you can alter during dev.

jinjagit commented 3 years ago

Also create a helper for the views to ask a helpful question like if delegating_supports|endorsements.

@davefrey Not sure I understand the delegating part. Why not just if supports|endorsements?

davefrey commented 3 years ago

The proposed config point, as I understand it, is used to decide "are we enabling delegation for Proposal Supports or Proposal Endorsements (or neither)?", for ex in the view where we're rendering the components.

The view could have if supports? but it's a little unclear to someone reading the code fresh.

Also if we want to handle all three cases, I think it reads better

if delegating_support?
  ... 
else if delegating_endorsements?
  ...
else
  ... classic decidim UI w/o delegation elements
jinjagit commented 3 years ago

Ahh, I see. I had assumed we were changing core Decidim Proposals functionality to totally either Supports or Proposals. i.e. You either see Supports + delegations, OR Endorsements + delegations.

I like your version better though.

So, we could have something like: delegation_mode, with 3 possible values: null / "supports" / "endorsements".

Maybe an explicit "off" is better than null ?

jinjagit commented 3 years ago

I propose reorganizing the plan to be:

By starting with creating the admin mode switch, this would enable me to keep the specs for the current 'supports' implementation, (by using a factory that includes the new mode), and thus ensure I don't break what already works as I develop the endorsements stuff. Thoughts?

davefrey commented 3 years ago

I think the admin switch should be later, because we still haven't had oliver input on our approach. I'd just hardcode it (you can just put your helpers in the controller and hardcode a boolean return)

jinjagit commented 3 years ago

I think the admin switch should be later, because we still haven't had oliver input on our approach. I'd just hardcode it (you can just put your helpers in the controller and hardcode a boolean return)

Boolean? We are proposing 3 states, no? Off, Supports, or Endorsements.

How do I write specs that use the controller hardcoding? How can I, for example, test that the 'Support' button does not get disabled when an Endorsements delegation is made.

davefrey commented 3 years ago

If it were me, I'd set off in this order:

  1. copy the delegate endorsement spec; use the happy path to drive outside-in the implementation. This would include hardcoding the :delegation_mode config point in the proposals controller
  2. get some team consensus on the Endorsements assumptions 3-5. do same as (1) for the other three calls (parallel to Supports)
  3. add db support (migration + rake task to fetch it), if we're doing :delegation_mode per proposal
  4. add admin and/or proposal component support as needed
davefrey commented 3 years ago

there are indeed three states. I propose the helper methods be boolean questions because it reads better in the (already semantically heavy) views. See code snippet above

jinjagit commented 3 years ago

I'm happy with sticking to the original plan order, but so far I haven't found a way to emulate different 'modes' when testing, as the hardcoded controller value seems always to override anything set in an rspec let or before statement.

oliverbarnes commented 3 years ago

So, here's my take: it's a safe to assume a user delegating their support for a proposal to a given person would also delegate their endorsement to them, or at least a vast majority of users would. And if we have the delegation simply work for both supports and endorsements, we avoid the whole admin configuration bit, and what's left to implement will be very similar to what was done for supports.

I think this would be a good-enough first version. If in the future we find that users actually have a strong demand for separating their delegations for support and endorsements, then we bite the bullet and implement that choice.

What do you think?

davefrey commented 3 years ago

Summarizing conversations from slack and pairing:

Extending the proposal_url is hacking around a data model mismatch between Decidim and Liquidvoting: Decidim conceptually has both a voteable thing (Proposal), and a vote method (Support, Endorse), but Liquidvoting has only a voteable thing (today this is explicitly a proposal_url). Our hack is to collapse the two Decidim things into the single LV thing.

Going through all the Decidim kinds of voting will be a nice test for our model, and that might be the right time to take what we learn to a LV refactoring. In any case, proposal_url as a name in LV is taking a beating :-)

oliverbarnes commented 3 years ago

I like where this is going :)

The issue of different voting methods was going to come up sooner or later, it was expected: down the line we'll have to tackle things like quadratic voting, for example. Endorsements vs supports for a given same proposal lets us start on this abstraction incrementally, since we simply need to distinguish between them, without changing any other mechanics on the API. We basically need separate tallies for each.

Instead of using a url hack, though, I'd go ahead and add a new voting_method graphql field to vote mutations and queries, and to voting results, on the API. A string field mapping to entries in a new voting_methods db table there, that gets upserted so we avoid the need for pre-seed or for an admin interface to manage it.

As for the delegations, I don't see the need for separate ones for each voting method at this point, for the reasons I explained on my last comment. I think it's Good Enough to have a single delegation for both supports and endorsements for a given proposal, and the user will be able to override that with their own support or endorsement. If the need for separate delegations arises, we can implement this later.

jinjagit commented 3 years ago

... A string field mapping to entries in a new voting_methods db table there, that gets upserted so we avoid the need for pre-seed or for an admin interface to manage it.

So, we would have a new voting_methods table on our LV api. This would map a vote.uuid to a method (support vs. endorse, for example). Would also map a voting_result.uuid to a method. Is this what you mean?

As for the delegations, I don't see the need for separate ones for each voting method at this point...

Agree.

davefrey commented 3 years ago

~I think we're still not aligned (which is normal), and we can discuss in person as three in our meeting tomorrow.~ (old text, I think this is cleared up now)

And I think stepping up to voting_method does simplify back to a single delegation. There is just one voteable thing, so one delegation. We'll need to change votingResult to either be a list, or include a count (etc) per voting method, counts will be different for Support and Endorse

jinjagit commented 3 years ago

.... new voting_method graphql field to vote mutations and queries, and to voting results, on the API

And this field would be optional? To allow our api to still work in it's purest, no voting method specified, way, as (for example) in the api readme examples. So we could use a create_vote api function, for example, that picks up the voting_method field if present and does the voting_method table stuff, and if no voting_method is specified the votes/results etc. are processed as they are now (without considering any voting_method.

oliverbarnes commented 3 years ago

@jinjagit:

So, we would have a new voting_methods table on our LV api. This would map a vote.uuid to a method (support vs. endorse, for example). Would also map a voting_result.uuid to a method. Is this what you mean?

Well, it would map to an association, perhaps like this on the Vote schema:

defmodule LiquidVoting.Voting.Vote do    
  schema "votes" do
    belongs_to :voting_methods, VotingMethods

Same for VotingResult.

And this field would be optional? To allow our api to still work in it's purest, no voting method specified, way, as (for example) in the api readme examples. So we could use a create_vote api function, for example, that picks up the voting_method field if present and does the voting_method table stuff, and if no voting_method is specified the votes/results etc. are processed as they are now (without considering any voting_method.

šŸ‘ agreed, it should be optional. To avoid splitting db queries we can just have an empty entry in the db - but this is an implementation detail we can figure out during, well, implementation

@davefrey:

or include a count (etc) per voting method, counts will be different for Support and Endorse

This one. One VotingResult per VotingMethod, through an association.

I'll include all of this in an issue I'll open next, on the api repo.

jinjagit commented 3 years ago

Well, it would map to an association, perhaps like this on the Vote schema:

Ahh, I see. And this table would list the different voting methods, which makes it easier to add more / see what they are / use them in other ways. :+1:

oliverbarnes commented 3 years ago

exactly :)

jinjagit commented 3 years ago

We should probably use "Decidim_proposal_supports" & "Decidim_proposal_endorsements", or something like that, as naming schemes for the voting_method then, to be really clear.

oliverbarnes commented 3 years ago

That's something to think about... though remember this would also be restricted by organization, like everything else. So entries wouldn't be shared between orgs that use different platforms, for instance. And within Decidim itself, afaik both supports and endorsements only exist in the context of a proposal.

jinjagit commented 3 years ago

Good point. I keep forgetting about the org level thing.

jinjagit commented 3 years ago

So, new plan is something like this:

All TDD, of course ;-) [I'll update the issues related to the proposed steps, if/when this agreed as OK]

oliverbarnes commented 3 years ago
  • [ ] Extend LV api to accept and use (optional) voting_method via createVote, deleteVote and calculateVotingResult GraphQl queries/mutations, including extension of all involved functions as appropriate / necessary (and adding new upsert_voting_method functionality).

šŸ‘ https://github.com/liquidvotingio/api/issues/225

  • [ ] Use LV api to record Decidim proposal supports as associated with respective voting_method, and update Decidim support count using the LV voting_result for this voting_method.

Covered by https://github.com/liquidvotingio/api/issues/225 as well

  • [ ] Ensure delegation/undelegation on Decidim proposals works smoothly with new LV voting_method for supports. (i.e. the 'correct' voting_result is updated accurately, when/if delegate supports/unsupports)

I don't foresee any change on the delegation part of things, but definitely on the results calculation. Also part of https://github.com/liquidvotingio/api/issues/225

  • [ ] Use LV api to record Decidim proposal endorsements as associated with respective voting_method, and update Decidim endorsement count using the LV voting_result for this voting_method.

šŸ‘ we need an issue for this here on the module repo

  • [ ] Ensure delegation/undelegation on Decidim proposals works smoothly with new LV voting_method for endorsements. (i.e. the 'correct' voting_result is updated accurately, when/if delegate endorses/unendorses)

same issue as above

All TDD, of course ;-)

As long as it doesn't block things :) it's ok to come up with code before tests when TDD is too slow.

oliverbarnes commented 3 years ago

This can be closed now that tasks have been hashed out.