[CPDNPQ-2210] Statement + contract setup #2116
Conversation
rwrrll
force-pushed
the
2210-contract-and-statement-setup
branch
from
58341fb to
e87c050
Compare
|
Review app deployed to https://npq-registration-review-2116-web.test.teacherservices.cloud/ |
rwrrll
force-pushed
the
2210-contract-and-statement-setup
branch
from
c25de36 to
9e451dc
Compare
| attr_accessor :error | ||
|
|
||
| def self.read(csv_file, row_class) | ||
| lines = CSV.read(csv_file, headers: true, encoding: "bom|utf-8") # encoding needed for some Excel CSVs |
| @@ -0,0 +1,5 @@ | |||
| class SetStatementDefaultReconcileAmount < ActiveRecord::Migration[7.1] | |||
| def change | |||
| change_column_default :statements, :reconcile_amount, from: nil, to: 0 | |||
There was a problem hiding this comment.
Pretty against this one - we shouldn't have a 'default amount' for a number we should be calculating - if its 0 then some code should have calculated its zero and set it?
nil shows nothing has calculated the amount
There was a problem hiding this comment.
Just to give the background here, it's not something the application ever calculates, it's a value that the finance team can optionally use to adjust the total of a statement (although the tool to actually set it doesn't exist (yet?))
It was added in 059377e "initial models for Statement and StatementItem" as part of separation, all existing statements had their reconcile_amount migrated over so the default was never a concern... however, now that we are creating new statements, this brings the column definition into parity with ECF, where it is:
t.decimal "reconcile_amount", default: "0.0", null: false
I can certainly see the argument in the other direction, but the reason I went with setting a default is that we will never set it automatically, and it won't necessarily be set/changed manually for every statement... Having said that we can certainly leave as nil if you prefer, if so I will modify the statement views to treat nil as 0, otherwise any statements we create (via this PR or otherwise) won't be viewable
| @@ -0,0 +1,22 @@ | |||
| class StrictDecimal < ActiveModel::Type::Integer | |||
rwrrll
force-pushed
the
2210-contract-and-statement-setup
branch
from
052cf8d to
a27a268
Compare
Feedback from @jebw
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Context
Ticket: https://dfedigital.atlassian.net/browse/CPDNPQ-2210
Ability to create statements (including contracts) for a cohort