-
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
Issue 11 Estimate next purchase date #29
Conversation
Visit the preview URL for this PR (updated for commit 3a52628): https://tcl-74-smart-shopping-list--pr29-sd-nr-11-c1v1e3jx.web.app (expires Fri, 20 Sep 2024 05:05:46 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 79d73546692f99aad8468c28e36db434e2c190ac |
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.
** 🦋 praise:** I tested two separate lists and six items, and not only is dateLastPurchased
no longer null, but dateNextPurchased
is accurately updating based on dateLastPurchased
and purchaseInterval
. The added documentation in the PR was very helpful.
Well done 🎊! Your work on this feature would make a great LinkedIn post 😉
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.
@NickRoccodev11 and @stacy-tech Great work! I am approving this code.
But, I have a question about how the purchaseInterval
is being updated. I added a new item with a purchaseInterval
of 14 and then I purchased that item from the List page.
When I checked data on the Firebase for that particular item, the purchaseInterval
was reset to 0.
Is this the expected behavior for the purchaseInterval
, or should it retain the previous value of 14 for that item?
Here is the workflow of how I tested it:
updated-screen.mov
@shuveksha-tuladhar Thanks for the video explanation! so yes, for now that is the expected behavior. the |
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.
Looks good and the behavior is working as expected! See the notes I added. Nothing is a blocker though.
@@ -10,3 +10,9 @@ const ONE_DAY_IN_MILLISECONDS = 86400000; | |||
export function getFutureDate(offset) { | |||
return new Date(Date.now() + offset * ONE_DAY_IN_MILLISECONDS); | |||
} | |||
export function getDaysBetweenDates(previousPurchaseDate) { |
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.
i would consider what to name this function - if i wanted to get the days between dates, i would expect this function to accept two values, date1 and date 2. Since you are only passing in one date and compare it to today, i would name this function something to reflect that.
This is so when this util function is needed for something else in the future it's easy to tell what it can do.
@@ -10,3 +10,9 @@ const ONE_DAY_IN_MILLISECONDS = 86400000; | |||
export function getFutureDate(offset) { |
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.
I would also reconsider making this function name more specific
src/api/firebase.js
Outdated
let daysSinceLastPurchase; | ||
|
||
if (dateLastPurchased) { | ||
daysSinceLastPurchase = getDaysBetweenDates(dateLastPurchased); | ||
} else { | ||
daysSinceLastPurchase = getDaysBetweenDates(dateCreated); | ||
} | ||
|
||
const daysUntilNextPurchase = calculateEstimate( | ||
purchaseInterval, | ||
daysSinceLastPurchase, | ||
totalPurchases, | ||
dateLastPurchased, | ||
); | ||
|
||
purchaseInterval = daysUntilNextPurchase; | ||
|
||
const dateNextPurchased = getFutureDate(daysUntilNextPurchase); |
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.
I think your logic makes sense here, however it is a little bit hard to follow (in nature due to the calculation itself being tricky. I would consider adding comments to each line to make sure it is clear what each line is doing.
This also feels like quite a big block of code. I would consider trying to separate out anything that is not the purpose of the updateItem
function (to update the database with the item's updated data), and seeing where else I could put this calculating dates logic - maybe in the list component itself, or another util function? You will then need to change the arguments for this function. This way we can keep this main function less noisy.
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.
Hi Jenny,
Thank you for your feedback! I appreciate your suggestion about adding comments to clarify the code. I’ve added comments to each line to explain what’s happening. This should make the logic more transparent.
Description
This ticket creates an estimate for the next purchase date of an item.
Related Issue
closes #11
Acceptance Criteria
When the user purchases an item, the item’s dateNextPurchased property is calculated using the calculateEstimate function and saved to the Firestore database
dateNextPurchased is saved as a date, not a number
A getDaysBetweenDates function is exported from utils/dates.js and imported into api/firebase.js
This function takes two JavaScript Dates and return the number of days that have passed between them
Updates
Before
After
Testing Steps / QA Criteria
dateNextPurchased
date.List
view.dateNextPurchased
has changed to a future date.