-
Notifications
You must be signed in to change notification settings - Fork 2
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
11. As an item, I want my estimated next purchase date to be computed at the time my purchase is recorded in the database, so the app can learn how often I buy different items #33
Conversation
Visit the preview URL for this PR (updated for commit feccffa): https://tcl-66-smart-shopping-list--pr33-an-hm-issue-11-uhmz7rd0.web.app (expires Wed, 13 Mar 2024 19:05:40 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2dc16cb2a79cbd6723fdc511b73e0743df1d18d0 |
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 reviewed your code, ran it locally, and checked firebase to confirm that dates were changing appropriately. I agree with writing the logic for calculating the dateLastPurchased and dateNextPurchased on the front end as it makes your code more readily understandable. Your code ran as expected. Great job on this complex issue!
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.
Everything here worked as expected for me. Great job with this, this a very clean concise solution to one of the most complicated issues on the project. Also thank you for providing clear testing instructions, as they were important for this PR.
One thing to consider: it might be nice, in my opinion, if users were able to un-check an item for a period of time after they first check it, to account for erroneously checking an item. Currently, if I check an item, it's stuck on "checked" and is logged as a purchase until enough time passes (I think 24 hours?). I actually believe as things stand there is no way to manually uncheck an item. This is not because of how you've coded your specific project, and following all the previous issues leads to this behavior.
Implementing something to change that would be completely optional and up to the team, but it's an idea I wanted to highlight.
const todaysDate = Timestamp.now(); | ||
|
||
// if dateLastPurchased is true subtract it from dateNextPurchased, else subtract dateCreated from dateNextPurchased to get the estimated number of days till next purchase | ||
const previousEstimate = Math.ceil( | ||
(dateNextPurchased.toDate() - |
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.
When I navigated to the Groceries list I got this error:
There were several items on the list that were created before the logic for all of the date fields (dateCreated, dateLastPurchased, etc) was implemented, so the previousEstimate
function was getting an undefined variable.
I do not think that this will be an issue now that the logic exists for all of the date fields. I am not sure, but it might be a good idea to add some error handling in case somehow one of these fields is not calculated? What do you think?
**note I deleted all the items in the Grocery Firebase Database that did not have calculated Date fields and the error didn't throw.
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.
BooYa! great work! I found an minor glitch due to development items being on a list (see comment on line 20). Once the items were deleted from the collection, it was solved. May be some error handling we want to consider down the road -- or may be completely void?
Description
We imported the
@the-collab-lab/shopping-list-utils
module to utilize the calculateEstimate function.In our
ListItem.jsx
we:previousEstimate
variable that checks if dateLastPurchased is true and subtracts it from dateNextPurchased, else it will subtract the dateCreated from dateNextPurchased to get the estimated number of days till next purchasedaysSinceLastTransaction
variable that checks if dateLastPurchased is true and subtracts it from todaysDate, else it will subtract the dateCreated from todaysDate to get the number of days since the last transactioncalculateEstimate
function from the@the-collab-lab/shopping-list-utils
module, utilizing the previousEstimate and daysSinceLastPurchase we calculated, and the total number of purchases of the itemupdateItem
functionIn
firebase.js
we:getFutureDate
function from our date utils and passed our nextPurchaseEstimate as the parameter to set our dateNextPurchasedWe put the logic for calculating the dateLastPurchased and or nextPurchase on the frontend instead of in firebase to avoid passing logic back and forth and creating clutter in the code. We were unsure if that is the most efficient or "best" route, and are open to feedback for better alternatives.
While testing, we found it difficult to check the exact math for the calculateEstimate function, but everything seemed to be right. Basically, that function takes the amount of days between purchases, averages them, and returns a whole number for the average amount of days between purchases.
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
Some notes
It felt like our acceptance criteria was based on previous logic for this project.
getDaysBetweenDates
is not a function in our utils, butgetFutureDate
was, so we wrote our logic/functionality utilizing that function instead. Because of this, we utilized the calculateEstimate function to give us the amount of days until our next purchase and the utilized the getFutureDate to convert the amount of days into our future date for purchase.Type of Changes
enhancement
Updates
Before
Not UI / Changes are in database
After
Not UI / Changes are in database
Testing Steps / QA Criteria