Sort items by urgency #57

kweeuhree · 2024-09-28T14:07:37Z

Description

This code proposes a way to sort items using sortByUrgency function and urgency state. The urgency status is passed to each item as a className.
I am using colors and borders to visualize the urgency, but it is a temporary solution:
overdue - dotted white border,
soon - green solid;
kindOfSoon - yellow thick border;
notSoon - solid blue thin border;
inactive - dashed red border;

The code does not pass tests, I do not know why. I faked return mock values, as it seems that I was not able to read dates.js module.

What isn't good:

I am relying on a global variable urgencyObject that holds the details temporarily before updating state with the details. Probably should be wrapped in a hook.
Inactivity of items is being hardcoded.
Sorting alphabetically doesn't work as expected.

Type of Changes

enhancement

Updates

After

Testing Steps / QA Criteria

Copious console logs show that items do get sorted

…te created; propose a function that will calculate days form passed in date to today; pass the latter into firebase

…n-order

…m/the-collab-lab/tcl-75-smart-shopping-list into wc-db-view-shopping-list-in-order

kweeuhree · 2024-09-28T14:08:45Z

.eslintrc.json

@@ -15,7 +15,7 @@
 	"settings": { "react": { "version": "detect" } },
 	"rules": {
 		"no-duplicate-imports": "warn",
-		"no-unused-vars": "warn",
+		"no-unused-vars": ["warn", { "argsIgnorePattern": "^_" }],
 		"react/jsx-filename-extension": [1, { "allow": "as-needed" }],


this was my solution as I had to loop over sets and didn't know how to avoid this here:

const getClassName = (name) => { const statusArray = Object.entries(urgency).find(([_, items]) => { return Array.from(items).some((item) => item.name === name); }); if (!statusArray) { throw new Error(`Failed to get class name of ${name}`); } return statusArray[0]; };

github-actions · 2024-09-28T14:08:46Z

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

https://tcl-75-smart-shopping-list--pr57-sort-by-urgency-jwz2o8i8.web.app

_{(expires Sat, 05 Oct 2024 14:08:38 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 1f1fd53c369e1fa31e15c310aa075b4e8f4f8dde}

kweeuhree · 2024-09-28T14:09:22Z

src/api/firebase.js

@@ -221,3 +225,74 @@ export async function deleteItem(listPath, itemId) {
 		throw new Error(`Failed updating item: ${error.message}`);
 	}
 }
+


global urgency object will be populated with details and later set urgency state within List component

It seems like leveraging context would be a great approach if the urgencyObject needs to be accessible across different components. Could we create a context for this and remove it from this file?

Agreed, it was in firebase.js as per acceptance criteria, but the logic for useUrgency is inside hooks.js!

- Updated `getItemUrgency` to calculate item urgency based on the time until the next purchase date in addition to the last purchase or creation date. - If more than 60 days have passed since the last purchase or creation, the item is considered inactive and assigned an urgency of 100. - If fewer than 60 days have passed, urgency is determined by the number of days until the next estimated purchase date.

ahsanatzapier · 2024-09-29T19:16:51Z

src/api/firebase.js

+const sortByUrgency = (item, daysUntilNextPurchase) => {
+	if (item.name.includes('sun')) {
+		console.log(`${item.name} urgencyStatus ${daysUntilNextPurchase}`);
+	}
+	if (daysUntilNextPurchase < 0) {
+		urgencyObject.overdue.add(item);
+		return;
+	} else if (daysUntilNextPurchase === 1000) {
+		urgencyObject.inactive.add(item);
+		return;
+	} else if (daysUntilNextPurchase < 7) {
+		urgencyObject.soon.add(item);
+		return;
+	} else if (daysUntilNextPurchase >= 7 && daysUntilNextPurchase < 30) {
+		urgencyObject.kindOfSoon.add(item);
+		return;
+	} else if (daysUntilNextPurchase >= 30) {
+		urgencyObject.notSoon.add(item);
+		return;
+	} else {
+		throw new Error(`Failed to place [${item.name}]`);
+	}
+};


This function appears to be purely client-side and doesn't interact with Firebase. Could we move it to a utility file and add a test to ensure its logic is covered?

ahsanatzapier · 2024-09-29T19:17:11Z

src/api/firebase.js

+const getItemUrgency = (item) => {
+	// get date the item was purchased or created
+	const itemDate = getDateLastPurchasedOrDateCreated(
+		item.dateLastPurchased,
+		item.dateCreated,
+	);
+	// check how many days have passed since that date
+	const daysToToday = getDaysFromDate(itemDate);
+
+	// if more than 60 days have passed
+	if (daysToToday >= 60) {
+		// sort as inactive
+		sortByUrgency(item, 1000);
+		return 1000;
+	} else {
+		// sort by the amount of days until next purchase date
+		const daysUntilNextPurchase = getDaysBetweenDates(
+			new Date(),
+			item.dateNextPurchased.toDate(),
+		);
+		if (item.name.includes('sun')) {
+			console.log(
+				`${item.name} days until next purchase is ${daysUntilNextPurchase}`,
+			);
+		}
+		sortByUrgency(item, daysUntilNextPurchase);
+
+		return daysUntilNextPurchase;
+	}
+};


This function appears to be purely client-side and doesn't interact with Firebase. Could we move it to a utility file and add a test to ensure its logic is covered?

ahsanatzapier · 2024-09-29T19:17:24Z

src/api/firebase.js

+export function comparePurchaseUrgency(item1, item2) {
+	const item1UrgencyStatus = getItemUrgency(item1);
+	const item2UrgencyStatus = getItemUrgency(item2);
+
+	if (item1UrgencyStatus === item2UrgencyStatus) {
+		console.log(`Sorting alphabetically: ${item1.name} vs ${item2.name}`);
+		return item1.name.localeCompare(item2.name);
+	}
+	// otherwise sort in descending order
+	return item1UrgencyStatus - item2UrgencyStatus;
+}


This function appears to be purely client-side and doesn't interact with Firebase. Could we move it to a utility file and add a test to ensure its logic is covered?

I will add tests!

ahsanatzapier · 2024-09-29T19:27:43Z

src/components/ListItem.jsx

@@ -17,7 +17,7 @@ const calculateIsPurchased = (dateLastPurchased) => {
 	return currentDate < oneDayLater;
 };

-export function ListItem({ item, listPath }) {
+export function ListItem({ item, listPath, className }) {


I see what you're trying to do here! Could I suggest a cleaner approach? Instead of passing a className, we should pass an urgencyStatus prop. This way, the className can be determined internally based on the urgencyStatus, keeping the styling logic encapsulated within the ListItem component. This approach is more semantic, makes the component more reusable, and keeps the parent component simpler.

Thank you! I am adding this as a change to useUrgency and use-urgency branch

ahsanatzapier · 2024-09-29T19:32:10Z

src/components/ListItem.jsx

+		<li className={`ListItem`}>
+			<div className={`urgency-status ${className}`} />


Once you've passed the urgencyStatus prop, you can use a switch statement like the one below to determine the appropriate class for rendering:

const getUrgencyClass = (urgencyStatus) => { switch (urgencyStatus) { case 'overdue': return 'urgency-overdue'; case 'soon': return 'urgency-soon'; case 'kindOfSoon': return 'urgency-kind-of-soon'; case 'notSoon': return 'urgency-not-soon'; case 'inactive': return 'urgency-inactive'; default: return ''; } };

Then, in the JSX:

<li className="ListItem"> <div className={`urgency-status ${getUrgencyClass(urgencyStatus)}`} /> </li>

As a bonus, you could even make getUrgencyClass into its own util to keep the code in this file cleaner!

I am not sure I understand, do we just add 'urgency-' to the class with this function? Is is about camel case? Or would it return a ui piece in the future?

ahsanatzapier · 2024-09-29T19:35:59Z

src/utils/dates.js

+export const getDaysFromDate = (pastDate) => {
+	const today = new Date();
+	const daysToToday = getDaysBetweenDates(pastDate, today);
+	return daysToToday;
+};


We can further optimize this by simplifying the function to:

export const getDaysFromDate = (pastDate) => getDaysBetweenDates(pastDate, new Date());

ahsanatzapier · 2024-09-29T19:37:57Z

src/utils/dates.js

+export const getDateLastPurchasedOrDateCreated = (
+	dateLastPurchased,
+	dateCreated,
+) => {
+	const lastPurchaseDate = dateLastPurchased?.toDate();
+	return lastPurchaseDate ?? dateCreated.toDate();
+};


We can further optimize this by simplifying the function to:

export const getDateLastPurchasedOrDateCreated = (dateLastPurchased, dateCreated) => dateLastPurchased?.toDate() ?? dateCreated.toDate();

ahsanatzapier · 2024-09-29T19:43:09Z

src/views/List.jsx

+	const [urgency, setUrgency] = useState(urgencyObject);
+	const listName = listPath.slice(listPath.indexOf('/') + 1);
+
+	useEffect(() => {
+		setUrgency(urgencyObject);
+	}, [urgencyObject]);
+
+	const getClassName = (name) => {
+		const statusArray = Object.entries(urgency).find(([_, items]) => {
+			return Array.from(items).some((item) => item.name === name);
+		});
+		if (!statusArray) {
+			throw new Error(`Failed to get class name of ${name}`);
+		}
+		return statusArray[0];
+	};


This logic will likely be replaced when we start passing the urgencyStatus prop directly to the component, removing the need for managing urgency locally.

ahsanatzapier · 2024-09-29T19:46:14Z

src/views/List.jsx

+	console.log('----');
+	console.log(`before filter: ${data.map((d) => d.name)}`);
+	const filteredItems = data
+		.filter((item) =>
+			item.name.toLowerCase().includes(searchItem.toLowerCase()),
+		)
+		.sort(comparePurchaseUrgency);

-	const filteredItems = data.filter((item) =>
-		item.name.toLowerCase().includes(searchItem.toLowerCase()),
-	);
-
-	const listName = listPath.slice(listPath.indexOf('/') + 1);
+	console.log(`filter: ${filteredItems.map((d) => d.name)}`);
+	console.log('----');
+	console.log(`Urgency state object:`);
+	console.log(urgency);


If we need to keep these console logs in production, can we put them behind a feature flag controlled by a query parameter in the URL? Perhaps something like ?debug=on.

The console logs are just to make testing more visual, I will remove before merging!

ahsanatzapier · 2024-09-29T19:48:27Z

src/views/List.jsx

+									key={item.id}
+									item={item}
+									listPath={listPath}
+									className={itemClassName}


This would be replaced by passing the urgencyStatus prop!

ahsanatzapier · 2024-09-29T19:49:22Z

tests/List.test.jsx

@@ -1,15 +1,29 @@
 import { render, screen } from '@testing-library/react';


Thanks for updating the test!

ahsanatzapier · 2024-09-29T19:51:25Z

tests/comparePurchaseUrgency.test.js

+describe('Compare Purchase Urgency', () => {
+	test('it works', () => {
+		expect(true).toBe(true);
+	});
+});


This seems like a placeholder. Could we remove it for now and add a meaningful test for the comparePurchaseUrgency function once it's ready?

You are right, it was very much a prototype, I will work on creating proper tests

ahsanatzapier · 2024-09-29T19:53:43Z

.eslintrc.json

@@ -15,7 +15,7 @@
 	"settings": { "react": { "version": "detect" } },
 	"rules": {
 		"no-duplicate-imports": "warn",
-		"no-unused-vars": "warn",
+		"no-unused-vars": ["warn", { "argsIgnorePattern": "^_" }],


Curious about the reasoning behind this change! Are there specific cases where we're intentionally using unused variables or function parameters prefixed with an underscore

I will remove this for the urgency hook! but this was my original getClassName function where I didnt know how to avoid an unused variable when looping over items within a set. In the urgency hook I managed though!
original reason:

const getClassName = (name) => { const statusArray = Object.entries(urgency).find(([_, items]) => { return Array.from(items).some((item) => item.name === name); }); if (!statusArray) { throw new Error(`Failed to get class name of ${name}`); } return statusArray[0]; };

warrenchan13 and others added 18 commits September 19, 2024 11:29

set up comparePurchaseUrgency function

0bdbbe9

set up test for comparePurchaseUrgency

6382ab2

add comparePurchaseUrgency to filteredItems

81fb444

set up logic for comparePurchaseUrgency function

3ac0feb

add a function that returns a valid date last purchased or a valid da…

57119a5

…te created; propose a function that will calculate days form passed in date to today; pass the latter into firebase

remove consolelog

8334eb1

Merge remote-tracking branch 'origin' into wc-db-view-shopping-list-i…

f552435

…n-order

comparePurchaseUrgency draft

8fad43a

fix: remove unused imports

ba509eb

feat: abstract getItemUrgency logic and remove comments

17b0aef

debug: add console logs for testing purposes

f251e3d

added item grouping draft

c76573d

fix typo on daysSinceLastPurchase

fd4951d

remove console log

0c0a2df

feat: display items based on urgency state

771545f

dont warn if _ is used

9e6f56c

add borders for testing

5d2ff4d

Merge branch 'wc-db-view-shopping-list-in-order' of https://github.co…

c92e940

…m/the-collab-lab/tcl-75-smart-shopping-list into wc-db-view-shopping-list-in-order

kweeuhree requested a review from warrenchan13 September 28, 2024 14:07

kweeuhree commented Sep 28, 2024

View reviewed changes

kweeuhree added 5 commits September 29, 2024 07:54

debug: update console logs to display dateNextPurchased

3ad3075

feat: update the way urgency status is displayed

b4f7a5f

debug: add a temporary solution to the test of List component

db073aa

feat: add overdue logic

8bb18a7

ahsanatzapier reviewed Sep 29, 2024

View reviewed changes

kweeuhree closed this Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort items by urgency #57

Sort items by urgency #57

kweeuhree commented Sep 28, 2024 •

edited

Loading

kweeuhree Sep 28, 2024

github-actions bot commented Sep 28, 2024

kweeuhree Sep 28, 2024

ahsanatzapier Sep 29, 2024

kweeuhree Oct 1, 2024

ahsanatzapier Sep 29, 2024

ahsanatzapier Sep 29, 2024

ahsanatzapier Sep 29, 2024

kweeuhree Oct 1, 2024

ahsanatzapier Sep 29, 2024

kweeuhree Oct 1, 2024

ahsanatzapier Sep 29, 2024 •

edited

Loading

kweeuhree Oct 1, 2024

ahsanatzapier Sep 29, 2024 •

edited

Loading

ahsanatzapier Sep 29, 2024

ahsanatzapier Sep 29, 2024

ahsanatzapier Sep 29, 2024

kweeuhree Oct 1, 2024

ahsanatzapier Sep 29, 2024

ahsanatzapier Sep 29, 2024

ahsanatzapier Sep 29, 2024

kweeuhree Oct 1, 2024

ahsanatzapier Sep 29, 2024

kweeuhree Oct 1, 2024

		<li className={`ListItem`}>
		<div className={`urgency-status ${className}`} />

		@@ -1,15 +1,29 @@
		import { render, screen } from '@testing-library/react';

Sort items by urgency #57

Sort items by urgency #57

Conversation

kweeuhree commented Sep 28, 2024 • edited Loading

Description

Type of Changes

Updates

After

Testing Steps / QA Criteria

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsanatzapier Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsanatzapier Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kweeuhree commented Sep 28, 2024 •

edited

Loading

ahsanatzapier Sep 29, 2024 •

edited

Loading

ahsanatzapier Sep 29, 2024 •

edited

Loading