-
Notifications
You must be signed in to change notification settings - Fork 0
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
Calculate rewards weighted by hours spent #23
Conversation
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.
It was quite a cursory glance! I guess if I was being difficult the 4 processes all look very similar could one field have been triggered with a different parameter to work with the different data or would that have sucked? Then a couple of small comments...
// console.debug('contributionsData:', contributionsData) | ||
|
||
// Iterate every week from the week of [Sun May-29-2022 → Sun Jun-05-2022] until now | ||
const weekEndDate = new Date('2022-06-05T00:00:00Z') |
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.
This had me scratching my head, because I am simple. Can we have a const like "the beginning of contribution time" defined somewhere and use that here instead, and how is it letting you change the time on the const date, the world is broken.
const theBeginningOfTime = new Date('2022-06-05T00:00:00Z')
let currentEndDate = theBeginningOfTime;
Go....
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.
Yeah, in all the other systems we use the week of 2022-05-22 as the beginning of time. So renaming would make sense. I made a separate issue for this: #24
} else { | ||
const reward = rewards[String(dataRow.passport_id)] | ||
reward.hours_spent_total += Number(dataRow.hours_spent) | ||
reward.hours_spent_total = Number(Number(reward.hours_spent_total).toFixed(2)) |
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.
This line is quite nuts, do you really have to do that?
let hours = +dataRow.hours_spent + dataRow.hours_spent_total hours = hours.toFixed(2) reward.hours_spent_total = hours
I dunnot maybe not worth it, jsut talk me while to read!
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.
This line is quite nuts, do you really have to do that?
let hours = +dataRow.hours_spent + dataRow.hours_spent_total hours = hours.toFixed(2) reward.hours_spent_total = hours
I dunnot maybe not worth it, jsut talk me while to read!
Hehe, I agree, nuts indeed 🤪
The reason for having it was just to store numbers as 1.95 instead of 1.95000000001. But I hope that there are more elegant solutions for solving this.
const reward = rewards[passportID] | ||
const rewardPercentage = 100 * reward.hours_spent_total / hoursSpentAllCitizens | ||
reward.reward_percentage = Number(rewardPercentage.toFixed(2)) | ||
}) |
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.
This is cool
@johnmark13 Yes, definitely. I made an issue for refactoring here: #25 |
close #7