Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 14, 2024
Merged

Issue 11 Estimate next purchase date #29

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93da49c
Implement future purcahse date tracker
NickRoccodev11 Sep 11, 2024
730f1a8
fix bugs with functions for updating dateNextPurchased
NickRoccodev11 Sep 12, 2024
3a52628
added comments to clarify the code
stacy-tech Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"npm": ">=8.19.0"
},
"dependencies": {
"@the-collab-lab/shopping-list-utils": "^2.2.0",
"@uidotdev/usehooks": "^2.4.1",
"firebase": "^10.12.5",
"react": "^18.3.1",
Expand Down
31 changes: 26 additions & 5 deletions src/api/firebase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import {
} from 'firebase/firestore';
import { useEffect, useState } from 'react';
import { db } from './config';
import { getFutureDate } from '../utils';

import { getFutureDate, getDaysBetweenDates } from '../utils';
import { calculateEstimate } from '@the-collab-lab/shopping-list-utils';
/**
* A custom hook that subscribes to the user's shopping lists in our Firestore
* database and returns new data whenever the lists change.
Expand Down Expand Up @@ -195,23 +195,44 @@ export async function addItem(listPath, { itemName, daysUntilNextPurchase }) {
dateNextPurchased: getFutureDate(daysUntilNextPurchase),
name: itemName,
totalPurchases: 0,
purchaseInterval: daysUntilNextPurchase,
});
}

export async function updateItem(
listPath,
{ itemId, totalPurchases, dateLastPurchased },
{ itemId, totalPurchases, dateLastPurchased, purchaseInterval, dateCreated },
) {
/**
* TODO: Fill this out so that it uses the correct Firestore function
* to update an existing item. You'll need to figure out what arguments
* this function must accept!
*/

const itemDocRef = doc(db, `${listPath}/items`, itemId);

await updateDoc(itemDocRef, {
let daysSinceLastPurchase;

if (dateLastPurchased) {
daysSinceLastPurchase = getDaysBetweenDates(dateLastPurchased);
} else {
daysSinceLastPurchase = getDaysBetweenDates(dateCreated);
}

const daysUntilNextPurchase = calculateEstimate(
purchaseInterval,
daysSinceLastPurchase,
totalPurchases,
dateLastPurchased,
);

purchaseInterval = daysUntilNextPurchase;

const dateNextPurchased = getFutureDate(daysUntilNextPurchase);
Copy link
Collaborator

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.

Copy link
Contributor

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.

await updateDoc(itemDocRef, {
totalPurchases: totalPurchases + 1,
dateLastPurchased: new Date(),
dateNextPurchased,
purchaseInterval,
});
}

Expand Down
9 changes: 6 additions & 3 deletions src/components/ListItem.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ export function ListItem({
itemId,
totalPurchases,
dateLastPurchased,
purchaseInterval,
dateCreated,
}) {
const [purchased, setPurchased] = useToggle(false);
const [isDisabled, setIsDisabled] = useState(false);

useEffect(() => {
if (dateLastPurchased) {
const checkExpiration = () => {
Expand Down Expand Up @@ -43,8 +44,10 @@ export function ListItem({
try {
await updateItem(listPath, {
itemId,
totalPurchases: totalPurchases + 1,
dateLastPurchased: new Date(),
totalPurchases,
dateLastPurchased,
purchaseInterval,
dateCreated,
});
console.log(`${name} updated successfully`);
alert(`${name} is purchased successfully`);
Expand Down
6 changes: 6 additions & 0 deletions src/utils/dates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,9 @@ const ONE_DAY_IN_MILLISECONDS = 86400000;
export function getFutureDate(offset) {
Copy link
Collaborator

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

return new Date(Date.now() + offset * ONE_DAY_IN_MILLISECONDS);
}
export function getDaysBetweenDates(previousPurchaseDate) {
Copy link
Collaborator

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.

const pastDate = previousPurchaseDate.toDate();
const presentDate = new Date();
const diffInMilliseconds = presentDate.getTime() - pastDate.getTime();
return Math.round(diffInMilliseconds / ONE_DAY_IN_MILLISECONDS);
}
5 changes: 2 additions & 3 deletions src/views/List.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import { useState } from 'react';
import { ListItem } from '../components';
// import { useNavigate } from 'react-router-dom';
import { NavLink } from 'react-router-dom';

export function List({ data, listPath }) {
const [searchInput, setSearchInput] = useState('');
//const navigate = useNavigate();

const handleInputChange = (e) => {
setSearchInput(e.target.value);
};
Expand Down Expand Up @@ -63,6 +60,8 @@ export function List({ data, listPath }) {
listPath={listPath}
totalPurchases={item.totalPurchases}
dateLastPurchased={item.dateLastPurchased}
purchaseInterval={item.purchaseInterval}
dateCreated={item.dateCreated}
/>
);
})
Expand Down