Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decouple the liquidvoting state object #85

Merged
merged 14 commits into from
Apr 2, 2021
Merged

Decouple the liquidvoting state object #85

merged 14 commits into from
Apr 2, 2021

Conversation

davefrey
Copy link
Member

This PR is meant to:

  1. centralize Liquidvoting logic into a module of helpers
  2. extract the Decidim-aware stuff like lv_state from our api client
  3. rename our client.rb to api_client.rb to make clearer boundaries and align with anticipated gem name

To get things working I've had to temporarily pin mimemagic to 0.3.9 directly in the Gemfile.lock file; this PR should not merge until we can have a clean bundle install-generated lock file.

Closes #48

davefrey added 6 commits March 31, 2021 12:32
@davefrey
Pinning mimemagic to 0.3.9
4bdab99 
I directly poked the Gemfile.lock, to get things running while we wait for Decidim to step up
to Rails 5.2.5.
@davefrey
Extract :update_with_lv_vote_count to a module helper
1211af5 
We're moving Liquidvoting-specific functionality into the Decidim::Liquidvoting module.

Logging needs be be added back in, coming with a new spec in next commit.

I would rather call `super` in `Proposal#update_votes_count` but the method is not found, idkw.

Specs are green.
@davefrey
Consistently use :update_votes_count
dc1bf4a
@davefrey
Test logging in our new :update_votes_count helper method
3761d17
@davefrey
Rename our client to api_client
497a4b0 
Align with our anticipated gem name
@davefrey
Pending spec for next time
65c23df
@davefrey davefrey marked this pull request as draft March 31, 2021 11:45
davefrey added 7 commits April 1, 2021 14:19
@davefrey
Introduce Liquidvoting.user_proposal_state
edd279b 
This also introduces via stub a new API call :fetch_user_supported
@davefrey
Add delegate_email to lv_state
2a7d7ff
@davefrey
Rename boolean query
46dd6f2
@davefrey
Cut over to new :user_proposal_state method
7a51718 
This also exposes the fetch queries needed to build a state object on the client side
@davefrey
Remove obsolete lv_state stuff from api_client
610de40
@davefrey
Consolidate the two ugly methods
ead5e32 
Finding if a user has voted or delegated is currently a brute-force filter of the whole LV result set.

This commit 1) consolidates the mess into two ugly private methods that need to be redone with
parameterized api calls, and 2) keeps the public method that delegates to the ugly privates.
@davefrey
Trying to fix CI gem install failure
bf1500f 
Used this one, to pick up nokogiri and rake: bundle update mimemagic --conservative

We're still not clean with our Gemfile.lock, waiting on a Decidim gem that solves mimemagic
@davefrey
Copy link
Member Author

davefrey commented Apr 1, 2021

I'm going to undraft this, but note the Gemfile.lock has been directly altered to pin a version of mimemagic as a workaround until Decidim releases new gems.

@davefrey davefrey marked this pull request as ready for review April 1, 2021 17:30
@oliverbarnes oliverbarnes changed the title 48 decouple the liquidvoting state object Decouple the liquidvoting state object Apr 1, 2021
@oliverbarnes
Copy link
Member

Great work 👏 It's almost there.

To get things working I've had to temporarily pin mimemagic to 0.3.9 directly in the Gemfile.lock file; this PR should not merge until we can have a clean bundle install-generated lock file.

Does 0.3.6 work? Because that's already merged on Decidim v0.24.0: decidim/decidim#7701 (see last item in https://github.com/decidim/decidim/releases/tag/v0.24.0)

If so, then we just need to bump to v0.24.0 (#68)

@davefrey
Copy link
Member Author

davefrey commented Apr 2, 2021

I'm trying a fresh v0.24.0 🤞 these gem tweaks are a pita

@davefrey
Copy link
Member Author

davefrey commented Apr 2, 2021

Bumping to v0.24.0 is bundle installing cleanly at mimemagic 0.3.10, so we're past that problem, but running rspec is giving segfaults for me on my M1 ... intel machine not handy today. Stay tuned

@oliverbarnes
Copy link
Member

I pulled your branch to try and replicate this, but specs ran fine for me. Steps I took:

  1. Pull branch
  2. Bundle update mimemagic
  3. bundle exec rake test_app
  4. bundle exec rspec

I'm on ruby 2.7.2 installed natively on my m1

@davefrey
Copy link
Member Author

davefrey commented Apr 2, 2021

See other PR 88 for just the 0.24 bump. PR coming for demo app.

I segfaulted on (my) 2.7.1, I was mirroring v0.24.0's ruby-version. But when I bumped back to 2.7.2, it ran cleanly. I suspect I built the two rubies differently, my new m1 machine has some odd things

oliverbarnes
oliverbarnes approved these changes Apr 2, 2021
View reviewed changes
Copy link
Member

@oliverbarnes oliverbarnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 very nice, thanks man

@oliverbarnes oliverbarnes merged commit 9a9f442 into master Apr 2, 2021
@oliverbarnes oliverbarnes deleted the 48-decouple-lv-state-object branch April 2, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverbarnes oliverbarnes oliverbarnes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decouple decidim api client from the decidim lv_state object
2 participants
@davefrey @oliverbarnes