-
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
Updating dateNextPurchased with smart calculation of new date #33
Conversation
Co-authored-by: Ross Clettenberg <[email protected]>
Co-authored-by: Ross Clettenberg <[email protected]>
Visit the preview URL for this PR (updated for commit 0b1fd59): https://tcl-77-smart-shopping-list--pr33-bb-rc-update-next-pu-qbfc9v9o.web.app (expires Sun, 22 Sep 2024 18:59:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72 |
Co-authored-by: Ross Clettenberg <[email protected]>
…ave to DB Co-authored-by: Ross Clettenberg <[email protected]>
Co-authored-by: Ross Clettenberg <[email protected]>
Co-authored-by: Ross Clettenberg <[email protected]>
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.
Great work y'all! Good use of the importing of utils and functions provided while also doing the math for getDaysBetweenDates
. I know back when I had worked on this card I had a hard time wrapping my head around the Timestamp dates by firebase needing to be converted with toDate()
for JS/TS. This is very clean!
My only suggestion is to remove the console and maybe paste it as a test case instruction in your PR for code reviewers to test in local. I understand the intention, and do love that you two thought that out to make it easier for us to test rather than calculate manually after reviewing firebase! However we shouldn't push console logs to production (just main in our case haha).
The console.logs have been removed before pushing to main
Description
getDaysBetweenDates
function taking 2 dates as parameters to calculate the whole number of days between dates.firebase
api to utilize thegetDaysBetweenDates
andcalculateEstimate
functions to use the items previousdateNextPurchase
,totalPurchases
and a calculateddaysSinceLastPurchased
to determine a new estimateddateNextPurchased
.dateNextPurchased
to store for future reference.Related Issue
Closes #11
Acceptance Criteria
dateNextPurchased
property is calculated using thecalculateEstimate
function and saved to the Firestore databasedateNextPurchased
is saved as a date, not a numbergetDaysBetweenDates
function is exported fromutils/dates.js
and imported intoapi/firebase.js
Type of Changes
Updates
No screenshots, focused on API changes that apply updates to the DB.
Testing Steps / QA Criteria
git pull origin bb-rc/update-next-purchase-date
and check that branch out with git checkoutbb-rc/update-next-purchase-date
npm ci
to install the newly added dependencies locally andnpm start
to launch the app.console.logs
above the variableupdates
onfirebase.ts
line 280.List
in the nav bar to navigate toconsole.logs
or thefirebase
if you have access to verify the changes indateNextPurchase