Use voting_method for voting, delegations & results #227

jinjagit · 2021-03-31T10:18:36Z

Aim: To pass voting_method via absinthe/GraphQl, and when it is included, record and use its value appropriately.

Addresses #225

Add to liquid_voting_web/schema. Now can be passed in via GraphQl.

Previous unique index would have prevented participant having two or more votes with different voting methods for same proposal.

Also fix typo in voting_methods test module name. 1 test failing - expected - as not yet including voting_method upsert in vote creation. Expect more to fail as dev continues, before finally fixing to reflect new methodology.

jinjagit · 2021-03-31T15:45:43Z

1 test failing - expected - as not yet including voting_method upsert in vote creation. Expect more to fail as dev continues, before finally fixing to reflect new methodology.

Creates voting_method record and association with vote. All kinds of voting related tests now breaking, as expected.

This disambiguates the field from the model/struct, and results in a selector: `voting_method.name`, rather than the confusing `voting_method.voting_method`.

This makes core voting tests pass, but core voting functions mostly do not yet make appropriet use of voting_method. Also rename absinthe schema field to match model field name. Also edit vote and voting_method factories to use new model field name.

Also edit get_vote!/2 to include |> Repo.preload([:voting_method]). Also edit test of get_vote!/2 to take advantage of preload.

Also add VotingMethods as Dataloader source in absinthe schema. Also add voting_method field to vote object in absinthe schema. These changes mean we can now return the values of associated voting_method fields in a votingMethod subgroup, when returning a query of a / some Vote(s).

Use Mutli.run in lib/liquid_voting_web/resolvers/voting.ex create_vote/3 funtion. Error handling may not yet cover all possible error matching.

Also remove all_fields var where is equivalent to required_fields.

Also add error field :action to help user understand which, of 3, actions involved in create_vote (upsert voting_method, upsert_participant, create_vote) gave rise to the error.

Merges in removal of unused vars in delegations test.

Also add :voting_method to telemetry :params in create_vote/1

All tests should be green at this point.

jinjagit · 2021-04-01T12:48:55Z

Summary of progress:

All tests currently green, though I expect to add more specific tests as work continues / when work is nearing completion.
Creation of vote now requires a voting_method parameter (at core and absinthe layers), which equates to voting_method.name.
votingMethod (and its fields and values) now exposed as associated with any existing vote(s) at absinthe layer = can be queried in query of vote(s)
results calculation does not yet involve voting_method
list_votes does not yet make use of voting_method

@oliverbarnes @davefrey This feels like a good point to get some feedback / ask & answer questions. I could participate in a meeting about this, if preferred.

Unfortunately, I cannot submit for review as merging this will break our Decidim demo, and I can't really fix that until the api is ready to work fully in voting_method mode.

davefrey · 2021-04-01T13:03:19Z

Hi Simon, congrats!

I can definitely review in the morning...I won't have the time to look carefully today.

You might describe more your approach and decisions in the PR description, that way I can understand your intent rather than inferring from the code: it will help me separate what you wanted to do from whether the code matches.

For example my first questions...again I can infer from the code, but tell us the story :-)

is this voting method required or do we have a default on the api (does the client have to send one)
is the voting result that we return for (almost) all mutators meant to include a count just for that voting method (just the supports count for example), or a comprehensive result for the proposal url (counts for endorsements and supports)?

Apologies if this is all laid out in previous slack or git issue conversations, ideally I'd be current on all that. And if your implementation intention is set out in the issue that's 👍 for me...closely reading the issue is part of PR review, and I'll get there.

jinjagit · 2021-04-01T13:20:17Z

@davefrey Thanks for the feedback! Here's some answers to your questions + some additional info:

* is this voting method required or do we have a default on the api (does the client have to send one)

Required when creating a vote. In discussion with Oliver, we decided this was the easiest way to avoid the complexities of trying to create a unique index against possible null values (do-able, but then creates Postgrex exception, which would need special handling, if an empty string is given for voting_method.name) & to also avoid explicitly setting a default string value. This means a voting_method is required even when a voting context (e.g. proposal_url) only has one 'method'.
Is associated with a result, and will be required when initiating a result calculation (not yet implemented).
May be required for list_votes, though I am currently undecided on this, as I can see pros and cons in having both a list_all_votes (of all voting methods) and a list_votes_for_voting method action.

* is the voting result that we return for (almost) all mutators meant to include a count just for that voting method (just the supports count for example), or a comprehensive result for the proposal url (counts for endorsements and supports)?

Voting results will be specific to a voting_method (and proposal_url).

Additional information:

The fundamental change is the creation of a VotingMethods table, for VotingMethod structs, with the fields :uuid and :name (string).

The VotingMethod struct is associated with each Vote, and will be associated with each Result.
These associations are / will be established by use of the core upsert_voting_method/1 function, called by the appropriate absinthe mutation paths (e.g. currently, it is added to the createVote path(s)).
These associations are / will also be used in the appropriate absinthe query paths (e.g. currently, it is used by the Vote query path).

oliverbarnes · 2021-04-01T14:15:50Z

This is looking good 🚀 I'll review again once the voting result part is done.

Good point about this only being mergeable when another PR on the module side consuming the new field is ready as well, we'll need to merge them both in tandem.

The only thing here that got me scratching my head so far is the updated index - I'm wondering if it's too granular. They have a cost in write time the bigger they get, and sometimes that outweighs the benefit.

But I honestly need to refresh up on this topic, so I can't say for sure. This is good homework for you, @jinjagit: reading up on composite index best practices and give us a tldr on current state of our indexing, and then revisit the indexing change in this PR in light of what you find

jinjagit · 2021-04-01T14:42:46Z

@oliverbarnes updated index - I'm wondering if it's too granular. They have a cost in write time the bigger they get, and sometimes that outweighs the benefit.

Thanks for the input. I'll look into it now.

Add voting_method(name) field to asbinthe deleteDelegation mutation, in schema and delete_delegation resolver function. Update doc comments for delete/delegation/5 core function.

Also remove debug output from existing_delegations_test.exs

proposalUrl and votingMethods fields are optional, but providing only a voting method, without a proposal url, will result in an error explaining that a proposal url is required IF a voting method is specified. If a proposal url is specified, with no voting method, then votes for the proposal url and voting method with name "default" will be returned. Providing neither will return all votes for an organization. An empty list is returned if no matching result(s) found.

New file: test/liquid_voting_web/absinthe/queries/votes_test.exs.

Also remove unused vars and comments.

jinjagit · 2021-04-08T17:48:06Z

Note: Because we decided to use a default value for votingMethod :name, all the examples in the readme still work without any need to edit them 👍

jinjagit · 2021-04-12T19:59:46Z

Video summary on YouTube
Video with transcript on Descript

@oliverbarnes @davefrey Hopefully, the video will help give an overview.

jinjagit · 2021-04-12T23:52:45Z

This should not break the demo if merged, since it will upsert votingMethod: "default" to our API database, for the current proposal support integration,and votingResults with no specified votingMethod should also return results for votingMethod: "default", without any need to edit the demo code and the graphQL mutations and queries it currently uses. 🤞

oliverbarnes

Thanks for all the work put into this, @jinjagit!

This is a big PR, but unavoidable due to the new voting_method being an almost global constraint, and much of the changes would break everything else if submitted separately.

Some of the api codebase was already due some refactoring and optimization, and some of the changes here increase that need a bit, but that tech debt is better tackled separately, in discrete and small PRs. They also take lower priority to moving ahead with integrating Decidim proposal endorsements, which depends on this PR.

I'm just going to note these small things here, for reference when tackling that:

Repo preloads can take collections of associations, so multiple calls to it can be merged into one
Some separate db queries might be more efficient if merged into a single one using joins
Some function signatures are getting pretty big
Some function are getting pretty complex, and this is already captured in some discussions here in the repo

And a subtle point: voting method sometimes comes as an argument in function signatures before proposal_url, when it's a constraint that most of the time will just use the default, and our domain it has a lower place - proposal, vote and delegation info pertain to the user actions (the user story), whereas voting method and organization id are more platform constraints - the user isn't choosing either in their voting and delegations, they're just given.

It's subtle but helps navigate the code if this hierarchy is clear and consistent :)

Going ahead and merging. Great work 👏

jinjagit · 2021-04-13T11:16:17Z

@oliverbarnes Great feedback. Thanks! I'll create a/some issue(s) to cover the points you make.

Pass voting_method into absinthe layer.

9c24ed0

Add to liquid_voting_web/schema. Now can be passed in via GraphQl.

jinjagit marked this pull request as draft March 31, 2021 10:18

jinjagit self-assigned this Mar 31, 2021

jinjagit added 3 commits March 31, 2021 11:24

Add voting_method to association to vote model changeset.

06cc2f3

Drop and create new votes unique index.

a662d76

Previous unique index would have prevented participant having two or more votes with different voting methods for same proposal.

Rename votes unique index and related changeset constraint.

57f3051

Also fix typo in voting_methods test module name. 1 test failing - expected - as not yet including voting_method upsert in vote creation. Expect more to fail as dev continues, before finally fixing to reflect new methodology.

jinjagit changed the title ~~Use voting_method for voting,~~ Use voting_method for voting Mar 31, 2021

jinjagit added 16 commits March 31, 2021 17:20

Upsert voting_method when create vote via GraphQl

ab662fd

Creates voting_method record and association with vote. All kinds of voting related tests now breaking, as expected.

Rename 'voting_method' field to 'name'.

a33002f

This disambiguates the field from the model/struct, and results in a selector: `voting_method.name`, rather than the confusing `voting_method.voting_method`.

Linting.

88b2caa

Use new field 'name' in absinthe schema and voting_method factory.

6b5dd5d

Create get_voting_method!/2 and related test.

0b521bf

Also edit get_vote!/2 to include |> Repo.preload([:voting_method]). Also edit test of get_vote!/2 to take advantage of preload.

Use Ecto.Multi to refactor nested case statements.

c09b5ba

Use Mutli.run in lib/liquid_voting_web/resolvers/voting.ex create_vote/3 funtion. Error handling may not yet cover all possible error matching.

Linting.

c35e517

Use correct model names in comments.

c6c4d02

Add :name to :voting_method required fields in changeset.

b9e4eb8

Also remove all_fields var where is equivalent to required_fields.

Combine 3 error matches into one.

7e2f1fa

Also add error field :action to help user understand which, of 3, actions involved in create_vote (upsert voting_method, upsert_participant, create_vote) gave rise to the error.

Merge branch 'master' into use-voting-method-for-voting.

3c4b890

Merges in removal of unused vars in delegations test.

Add voting_method to create_vote test in voting_test.exs.

dfe51b4

Also add :voting_method to telemetry :params in create_vote/1

Add upsert voting method to absinthe create vote with participant id

006f427

Add voting_method field to absinthe create_vote tests.

294119d

All tests should be green at this point.

jinjagit mentioned this pull request Apr 1, 2021

Review composite indexing #229

Open

jinjagit added 7 commits April 7, 2021 20:30

Improve doc comments for create_delegation/1

1d95b34

Fix typos in comments.

6389505

Use voting_method when deleting a delegation.

bccca21

Add voting_method(name) field to asbinthe deleteDelegation mutation, in schema and delete_delegation resolver function. Update doc comments for delete/delegation/5 core function.

Use voting_method in absinthe delete delegations test.

23617de

Also remove debug output from existing_delegations_test.exs

Improve comments and code layout.

7d6d91c

Remove unused vars.

cff7abc

jinjagit mentioned this pull request Apr 8, 2021

Create/delete delegation do not update the vote count liquidvotingio/decidim-module-liquidvoting#94

Closed

jinjagit added 2 commits April 8, 2021 16:54

Add absinthe tests of Votes query.

94a5fcc

New file: test/liquid_voting_web/absinthe/queries/votes_test.exs.

Fix incorrect test file module name.

935e074

Also remove unused vars and comments.

Use voting_method in absinthe voting results subscription.

2650657

jinjagit changed the title ~~Use voting_method for voting~~ Use voting_method for voting, delegations & results Apr 10, 2021

jinjagit marked this pull request as ready for review April 12, 2021 22:11

jinjagit requested review from oliverbarnes and davefrey April 12, 2021 22:12

jinjagit marked this pull request as draft April 12, 2021 22:21

jinjagit removed request for oliverbarnes and davefrey April 12, 2021 22:21

jinjagit marked this pull request as ready for review April 12, 2021 23:53

jinjagit requested review from oliverbarnes and davefrey April 12, 2021 23:54

oliverbarnes approved these changes Apr 13, 2021

View reviewed changes

oliverbarnes merged commit 9aed184 into master Apr 13, 2021

oliverbarnes deleted the use-voting-method-for-voting branch April 13, 2021 11:07

This was referenced Apr 14, 2021

Prevent votingResult query for absinthe global createDelegation #232

Closed

Add voting methods #225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use voting_method for voting, delegations & results #227

Use voting_method for voting, delegations & results #227

jinjagit commented Mar 31, 2021 •

edited by oliverbarnes

Loading

jinjagit commented Mar 31, 2021

jinjagit commented Apr 1, 2021 •

edited

Loading

davefrey commented Apr 1, 2021

jinjagit commented Apr 1, 2021 •

edited

Loading

oliverbarnes commented Apr 1, 2021

jinjagit commented Apr 1, 2021

jinjagit commented Apr 8, 2021

jinjagit commented Apr 12, 2021 •

edited

Loading

jinjagit commented Apr 12, 2021 •

edited

Loading

oliverbarnes left a comment

jinjagit commented Apr 13, 2021 •

edited

Loading

Use voting_method for voting, delegations & results #227

Use voting_method for voting, delegations & results #227

Conversation

jinjagit commented Mar 31, 2021 • edited by oliverbarnes Loading

jinjagit commented Mar 31, 2021

jinjagit commented Apr 1, 2021 • edited Loading

davefrey commented Apr 1, 2021

jinjagit commented Apr 1, 2021 • edited Loading

oliverbarnes commented Apr 1, 2021

jinjagit commented Apr 1, 2021

jinjagit commented Apr 8, 2021

jinjagit commented Apr 12, 2021 • edited Loading

jinjagit commented Apr 12, 2021 • edited Loading

oliverbarnes left a comment

Choose a reason for hiding this comment

jinjagit commented Apr 13, 2021 • edited Loading

jinjagit commented Mar 31, 2021 •

edited by oliverbarnes

Loading

jinjagit commented Apr 1, 2021 •

edited

Loading

jinjagit commented Apr 1, 2021 •

edited

Loading

jinjagit commented Apr 12, 2021 •

edited

Loading

jinjagit commented Apr 12, 2021 •

edited

Loading

jinjagit commented Apr 13, 2021 •

edited

Loading