Conversation
|
Hello Tamir. Could you try to add a bit more context in the description of the PR. The description is a bit hard to read, and is good to add a section explaining how to test this (steps), this makes the review easier and reduce the chance of misunderstanding the scope and goal of the PR |
anxolin
left a comment
anxolin
left a comment
There was a problem hiding this comment.
The description of the PR is way better now. Thanks for applying the changes
| } | ||
|
|
||
| // Helper function to calculate months difference between two dates | ||
| function getMonthsDifference(startDate: Date, endDate: Date): number { |
There was a problem hiding this comment.
This belong to a utils file, in case you need it in another part of the project you don't want to add a dependency to this file
| // For create operation, we handle it in afterCreate since we need the ID | ||
| } | ||
|
|
||
| async function calculateServiceFeeEnabled(solver: Solver, data: SolverData): Promise<void> { |
There was a problem hiding this comment.
why is this called calculate, but returns nothing?
| // For create operation, we handle it in afterCreate since we need the ID | ||
| } | ||
|
|
||
| async function calculateServiceFeeEnabled(solver: Solver, data: SolverData): Promise<void> { |
There was a problem hiding this comment.
Would be positive to add some minmial JsDoc to explain what is the responsability of the function
| } | ||
|
|
||
| async function calculateServiceFeeEnabled(solver: Solver, data: SolverData): Promise<void> { | ||
| data.isServiceFeeEnabled = false; |
There was a problem hiding this comment.
this is mutating the parameter, is this desired?
There was a problem hiding this comment.
Yes, if its not set to true it needs to be false by default
| const joinedDate = new Date(bondingPool.joinedOn); | ||
| const monthsDifference = getMonthsDifference(joinedDate, currentDate); | ||
|
|
||
| if (bondingPool.name === "CoW" && solver.isColocated === "No") { |
There was a problem hiding this comment.
Why name CoW, its hard to me to reason about this logic
There was a problem hiding this comment.
This is the bonding pool name in the referenced table (an existing entry)
| const monthsDifference = getMonthsDifference(joinedDate, currentDate); | ||
|
|
||
| if (bondingPool.name === "CoW" && solver.isColocated === "No") { | ||
| if (monthsDifference >= 6) { |
| } | ||
| // Colocated bonding pool | ||
| else if (solver.isColocated === "Yes") { | ||
| if (monthsDifference >= 3) { |
| ); | ||
|
|
||
| if (solver) { | ||
| await calculateServiceFeeEnabled(solver as Solver, solverData); |
There was a problem hiding this comment.
it feels like you want to update something and first you want to do some calculations. If that's the case, is better to make a pure function to decide the new value and then do the update
This function relies on side effects and does too many things. Follow SOLID single responsability principle "https://www.digitalocean.com/community/conceptual-articles/s-o-l-i-d-the-first-five-principles-of-object-oriented-design#single-responsibility-principle
2 participants
As part of our recent efforts to improve data integrity in the CMS we are adding event hooks to certain actions in the cms that update other fields, this way we ensure that data is always up to date in realtime unlike now when fields are not updated because of manual mistakes or things changing in one table but not reflected in another.
In this pr we add some simple lifecycles to the solver table:
before create or update operations on the solver table:
To test this change: