-
Notifications
You must be signed in to change notification settings - Fork 1
Added lifecycle hooks to solver table #85
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
Changes from 3 commits
5892d45
a7e2fb4
7ea0948
b15c260
f36bff8
c41513f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| import { errors } from "@strapi/utils"; | ||
|
|
||
| export default { | ||
| async beforeCreate(event) { | ||
| try { | ||
| await updateActiveNetworks(event); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be done after updating or creating? |
||
| await updateServiceFeeEnabled(event); | ||
| } catch (error) { | ||
| console.error("Error in beforeCreate:", error); | ||
| throw new errors.ValidationError(error.message || "Error processing solver data"); | ||
| } | ||
| }, | ||
|
|
||
| async beforeUpdate(event) { | ||
| try { | ||
| await updateActiveNetworks(event); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since these updates and creates will be done through Dagster, what's the advantage of processing this through the lifecycle vs adding this update as a step in the same Dagster job that will do the update that triggers this hook? |
||
| await updateServiceFeeEnabled(event); | ||
| } catch (error) { | ||
| console.error("Error in beforeUpdate:", error); | ||
| throw new errors.ValidationError(error.message || "Error processing solver data"); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| interface SolverData { | ||
| activeNetworks?: string[]; | ||
| hasActiveNetworks?: boolean; | ||
| isServiceFeeEnabled?: boolean; | ||
| solver_networks?: any; | ||
| } | ||
|
|
||
| interface StrapiEvent { | ||
| params: { | ||
| data: SolverData; | ||
| where?: { | ||
| id: number; | ||
| }; | ||
| }; | ||
| result?: { | ||
| id: number; | ||
| }; | ||
| } | ||
|
|
||
| async function updateActiveNetworks(event: StrapiEvent) { | ||
| const { data, where } = event.params; | ||
| const solverData: SolverData = data; | ||
|
|
||
| if (where && !data.solver_networks) { | ||
| try { | ||
| const solver = await strapi.entityService.findOne( | ||
| 'api::solver.solver', | ||
| where.id, | ||
| { populate: ['solver_networks.network'] } | ||
| ); | ||
|
|
||
| if (solver) { | ||
| await calculateActiveNetworks(solver as Solver, solverData); | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error fetching solver data for id ${where.id}:`, error); | ||
| throw new errors.ApplicationError(`Failed to fetch solver data: ${error.message}`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| interface Solver { | ||
| id: number; | ||
| solver_networks?: Array<{ | ||
| active?: boolean; | ||
| network?: { | ||
| name?: string; | ||
| }; | ||
| }>; | ||
| solver_bonding_pools?: Array<{ | ||
| name?: string; | ||
| joinedOn?: string; | ||
| }>; | ||
| isColocated?: string; | ||
| } | ||
|
|
||
| async function calculateActiveNetworks(solver: Solver, data: SolverData): Promise<void> { | ||
| if (!solver.solver_networks) { | ||
| data.activeNetworks = []; | ||
| data.hasActiveNetworks = false; | ||
| return; | ||
| } | ||
|
|
||
| // Filter active networks and extract their names | ||
| const activeNetworkNames = solver.solver_networks | ||
| .filter(network => network.active) | ||
| .map(network => network.network?.name) | ||
| .filter(Boolean) as string[]; // Remove any undefined values | ||
|
|
||
| data.activeNetworks = activeNetworkNames; | ||
| data.hasActiveNetworks = activeNetworkNames.length > 0; | ||
| } | ||
|
|
||
| async function updateServiceFeeEnabled(event: StrapiEvent): Promise<void> { | ||
| const { data, where } = event.params; | ||
| const solverData: SolverData = data; | ||
|
|
||
| if (where) { | ||
| try { | ||
| const solver = await strapi.entityService.findOne( | ||
| 'api::solver.solver', | ||
| where.id, | ||
| { | ||
| populate: { | ||
| solver_bonding_pools: { | ||
| fields: ['name', 'joinedOn'] | ||
| } | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| if (solver) { | ||
| await calculateServiceFeeEnabled(solver as Solver, solverData); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All comments showed outdated code and I couldn't find the original function. When you address a comment and code changes a lot so GH can placed a discussion in the right place of the new code, is good you include a link to the commit you pushed to address the comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, you move part of Also, it doesn't So it could be something like: async function updateActiveNetworks(solver: Solver, data: SolverData): Promise<void> {
const activeNetworks = getActiveNetworks(solver.solver_networks)
data.hasActiveNetworks = activeNetworks.length > 0;
data.activeNetworks = activeNetworks ? activeNetworks.map(network => network..name) : '';
}Not only this is simpler and more readable than the current code, but also, if you remove a network, you won't get the value you expect because of how you return early (you don't update the data if there's no network info). Original code: async function calculateActiveNetworks(solver: Solver, data: SolverData): Promise<void> {
if (!solver.solver_networks) {
data.activeNetworks = [];
data.hasActiveNetworks = false;
return;
}
// Filter active networks and extract their names
const activeNetworkNames = solver.solver_networks
.filter(network => network.active)
.map(network => network.network?.name)
.filter(Boolean) as string[]; // Remove any undefined values
data.activeNetworks = activeNetworkNames;
data.hasActiveNetworks = activeNetworkNames.length > 0;
} |
||
| } | ||
| } catch (error) { | ||
| console.error(`Error fetching solver data for service fee calculation (id ${where.id}):`, error); | ||
| throw new errors.ApplicationError(`Failed to fetch solver data for service fee calculation: ${error.message}`); | ||
| } | ||
| } | ||
| // For create operation, we handle it in afterCreate since we need the ID | ||
| } | ||
|
|
||
| async function calculateServiceFeeEnabled(solver: Solver, data: SolverData): Promise<void> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this called calculate, but returns nothing?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be positive to add some minmial JsDoc to explain what is the responsability of the function |
||
| data.isServiceFeeEnabled = false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is mutating the parameter, is this desired?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if its not set to true it needs to be false by default |
||
|
|
||
| if (!solver.solver_bonding_pools || solver.solver_bonding_pools.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const currentDate = new Date(); | ||
|
|
||
| for (const bondingPool of solver.solver_bonding_pools) { | ||
| if (!bondingPool.joinedOn) { | ||
| continue; | ||
| } | ||
|
|
||
| const joinedDate = new Date(bondingPool.joinedOn); | ||
| const monthsDifference = getMonthsDifference(joinedDate, currentDate); | ||
|
|
||
| if (bondingPool.name === "CoW" && solver.isColocated === "No") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why name CoW, its hard to me to reason about this logic
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the bonding pool name in the referenced table (an existing entry)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic seems to fragile to me. So you are following an undocumented naming convention, that is hidden in a method of this lifecycle. Also, this method suffers from the same things I mentioned here #85 (comment)
|
||
| if (monthsDifference >= 6) { | ||
|
anxolin marked this conversation as resolved.
Outdated
|
||
| data.isServiceFeeEnabled = true; | ||
| return; | ||
| } | ||
| } | ||
| // Colocated bonding pool | ||
| else if (solver.isColocated === "Yes") { | ||
| if (monthsDifference >= 3) { | ||
|
anxolin marked this conversation as resolved.
Outdated
|
||
| data.isServiceFeeEnabled = true; | ||
| return; | ||
| } | ||
| } | ||
| // For partial colocated, we'll treat it as not colocated | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error calculating service fee enabled status:`, error); | ||
| throw new errors.ApplicationError(`Failed to calculate service fee status: ${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| // Helper function to calculate months difference between two dates | ||
| function getMonthsDifference(startDate: Date, endDate: Date): number { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| const years = endDate.getFullYear() - startDate.getFullYear(); | ||
| const months = endDate.getMonth() - startDate.getMonth(); | ||
| return years * 12 + months; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those lifecycle hooks are applied to
solvercollection; shouldn't they be applied tosolver-networkcollection?For example, you have a
solveritem andsolver-networkwhich are coupled. Once you delete thesolver-networkitem, it should update thesolveritem fields.