the-collab-lab / tcl-28-smart-shopping-list

1 stars 3 forks source link

Sd ia estimate next purchase #32

Closed sandaiiyahh closed 3 years ago

sandaiiyahh commented 3 years ago

For an example of how to fill this template out, see this Pull Request.

Description

Thoughts We Had:

UPDATE:

Related Issue

closes #10

Acceptance Criteria

Type of Changes

Type
:bug: Bug fix
āœ“ :sparkles: New feature
āœ“ :hammer: Refactoring
:100: Add tests
:link: Update dependencies
:scroll: Docs

Updates

Before

Screen Shot 2021-08-10 at 9 01 33 PM

After

Variables are initially declared when you first add an item:

Screen Shot 2021-08-10 at 9 02 14 PM

After checking off an item as purchased:

Screen Shot 2021-08-10 at 9 03 07 PM

After an item has been purchased more than 2 times:

Screen Shot 2021-08-10 at 9 04 22 PM

Testing Steps / QA Criteria

github-actions[bot] commented 3 years ago

Visit the preview URL for this PR (updated for commit daff347):

https://tcl-28-shopping-list--pr32-sd-ia-estimate-next-atpbv9td.web.app

(expires Fri, 20 Aug 2021 23:24:31 GMT)

šŸ”„ via Firebase Hosting GitHub Action šŸŒŽ

meganesu commented 3 years ago

Nicely done y'all! Like others have said, this was a complicated ticket, and you did a great job sticking with it! Well done šŸ„³

A few thoughts in response to @luisaugusto's review: (happy to discuss more tomorrow in Slack!)

There is one issue where if you uncheck an item because you accidentally clicked, there is no way to undo the calculation that you already made, so you can keep toggling the checkbox to alter the data for the item with no way to revert.

Personally, in the interest of time (since I know y'all have already spent quite a while working on this), I'd suggest we create another story to handle undoing accidental purchase calculations, since that functionality is not explicitly included in the acceptance criteria for this ticket.

I noticed that there are currently three fields in Firebase: isPurchased, lastPurchasedDate, and numberOfPurchases. What I would recommend doing is consolidating those three fields into one purchaseDates array.

Similarly, I got the impression from @segdeha's comment in Slack that storing a list of all the purchase dates is also out of scope.

For older items in lists that were made before this change, the numberOfPurchases field is returning NaN, it would be good to check this and default it to 0 if it's missing in an older item without the field.

Agreed, adding a quick check and default value is a good idea. During office hours, we'd debated dumping the existing documents in the database and starting from scratch (since it's a small project and we don't have any real users yet). But in a real professional setting, that wouldn't be an option, since odds are you'd already have a bunch of user data in the database.

isaabutaa commented 3 years ago

Hey @meganesu @luisaugusto thank-you both for the feedback and suggestions. Since it was rather small, I committed the change that Luis suggested: