forked from zinc-collective/convene
-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
✨ Tobias
: Issuing a Payout
#15
Open
zspencer
wants to merge
13
commits into
main
Choose a base branch
from
tobias/issuing-payouts
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zspencer
changed the title
✨
✨ Jan 30, 2024
Tobias
: Begin Issuing a Payout
Tobias
: Model Issuing a Payout
zspencer
force-pushed
the
tobias/issuing-payouts
branch
from
January 30, 2024 18:12
5bf375c
to
6cca03d
Compare
zspencer
force-pushed
the
tobias/issuing-payouts
branch
2 times, most recently
from
February 12, 2024 22:25
bd9562f
to
a0ee024
Compare
- #11 I'm starting out by coding by wishful thinking, putting together the pieces of the pie that I can eat bite by bite. In this test, I'm using imagining there will be a Model called `Tobias::Payout`, which will be our computational entrance point into the [Issuing a Payout](#11) feature. I considered starting with a system test, which would have gone through the User Interface and tied together a bunch of different concepts; but I figured I would start with a `model` spec; so that I can stay focused on drawing the computer-facing side of the feature out before worrying too much about the human-facing bits; which I always find require a lot more thought for me. That said, a system test will be useful here at some point. The computational bits, on the other hand, feel pretty accesible. We want to create Payments for every Beneficiary of the appropriate amount, and store records of these Payments so we know who is supposed to get paid what. Here's a line-by-line play-by-play of this change: I'm using the `spec` folder to store executable examples of a feature. Because `Issuing Payout` is a feature for the [`TOBIAS` project](#1), I am storing it under `spec/tobias`. My choice of what to call the file (`payout_spec.rb` indicates that this spec will be for the `Payout` model. The `require "rails_helper"` line tells our testing framework to load all the code necessary to run a spec. The `describe Tobias::Payout do` line groups the examples nested within it as relating to the `Payout` model. The `describe "#issue"` line tells me that the `Payout` model will have a method named `issue`, and also creates a group of examples that describe how the `Payout#issue` method works. The `it "issues a Payment..." do` line describes one of the examples we we plan to use to confirm that the `Payout#issue` method works the way we hope it will. The test itself lives on the lines between `it "issues a Payment..." do` and the `end` that is aligned with the `it` The `payout = create(:tobias_payout, payout_amount_cents: 150_00) line says "create a database record of a `Tobias::Payout, and populate it's `payout_amount_cents` field with $150.00, and store a reference to it in the `payout` variable. The `beneficiaries = create_list(10, :tobias_beneficiary, trust: payout.trust)` line says create 10 `Tobias::Beneficiary` records in the database, and make sure their `trust` field is pointed at the same `Tobias::Trust` that our `payout` is pointing to. Store references to those in the `beneficiaries variable.` The `payout.issue` line executes the `Payout#issue` method that we are describing. The `beneficiaries.each do |beneficiary|` goes over every one of the newly created `Tobias::Beneficiary` records stored in the `beneficiaries` variable, expose each one as a variable named `beneficiary`, and execute the code between it and the next `end`. The `expect(beneficiaries.payments).to exist(amount_cents)` tells our example to let us know if there are no `Tobias::Payment` records in the database for one of the `beneficiaries`.
This sketches in the `Payout#issue` method's line-of-action, which is to iterate through the `Payout#trust#beneficiaries` and add a `Payment` to the set of `Payout#payments` for the per-beneficiary amount. That said, there's a few outstanding questions we'll probably want to add some tests to interrogate, especially around: 1. What happens if the method is ran twice with the same data? 2. What about if the set of `Payout#trust#beneficiaries` is changed between calls to `Payout#issue`? 3. What happens when the `Payout#payout_amount` doesn't divide evenly between the `Payout#beneficiaries?
There was a number of things I didn't love about my implementation of `Payout#issue`: - I had added a `Tobias::Record` far earlier than necessary - There were linter errors - `Payout#payout_amount` felt redundant This remediates those nose-wrinkles, and sets us up nicely for the next test.
…Beneficiary` `Payment` down I wasn't exactly sure what would happen, so I took the time to figure it out. Apparently, the [`money-rails` gem] we use defaults to rounding down, which is the safer thing to do. [`money-rails` gem]: https://github.com/RubyMoney/money-rails
…s` (#19) * 🥗 `Tobias`: Issuing `Payouts` doesn't create additional payouts per beneficiary I thought that this would actually be a problem, but apparently it's not! Hooray for tests! * 🥗✨`Tobias`: `Payout#issue` won't create `Payment` for new `Beneficiary` This one actually needed adjusting! Turns out, `Payout#issue` would happily create a new `Payment` for a new `Beneficiary`, which is less than ideal. Now we have a guard clause that will prevent `Payout#issue` from making additional `Payments` when there are already `Payments` present.
While it may be better us to void a `Payout`, making sure we clean up any `Payout#payments` upon `Payout#destroy` should reduce the likelihood of data being in a sad place.
This is not super important to the actual feature, but it does mean that the `Payment` has clearly defined relationships, with tests that confirm the relationship. It also adds a spec to check that the `#amount` value operates as a monetized value; which doesn't seem like it does that much beyond giving us an injection point for layering in additional details.
A few more `Payout` bits that didn't have specs! Now they do!
Again, we may want to leverage an archival system rather than destruction, but for now let's make sure we clean up when a `Trust` is detroyed.
zspencer
force-pushed
the
tobias/issuing-payouts
branch
from
March 25, 2024 21:25
6ba9035
to
b1b3055
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tobias
: Issuing aPayout
#11Enhancements
Payout#issue
via WebPayout
on WebTests
Beneficiary
Payout
Payment
Trust
Tidying
dependent
andinversed
Open Questions
Payout#issue
runs twice with the samePayout#amount
andPayout#trust#beneficiaries
?Payout#trust#beneficiaries
is changed between calls toPayout#issue
?Payout#amount
doesn't divide evenly betweenPayout#beneficiaries
?