Skip to content

ERA-10594: Leverage read filter param in messages API calls from das-web-react #1259

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

Merged
merged 4 commits into from
Mar 26, 2025
Merged

ERA-10594: Leverage read filter param in messages API calls from das-web-react #1259

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
985be85
Adding read:false to get message duck
Mar 21, 2025
1537b5f
Adding new duck to handle fetch of unread message count
Mar 24, 2025
26066ef
Adding feedback PR
Mar 26, 2025
18408f8
Reset files
Mar 26, 2025
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
16 changes: 11 additions & 5 deletions src/Nav/MessageMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,21 @@ const MessageMenu = () => {
[state.results.length, subjects]
);

const fetchMenuMessages = useCallback(() => {
fetchAllMessages({ page_size: 100 })
.then((results = []) => dispatch(fetchMessagesSuccess({ results })))
const fetchMenuMessages = useCallback((getUnreadMessagesOnly = false) => {
const page_size = 100;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a top of the file constant MENU_MESSAGES_PAGE_SIZE 😄

const params = !getUnreadMessagesOnly ? { page_size } : { page_size, read: false };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I believe We only want to use read:false to calculate the badge count I'm building the params conditionally since fetchMenuMessages is being used in several places of the component tree of this file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the condition could be simplified to:

fetchAllMessages({ page_size: MENU_MESSAGES_PAGE_SIZE, read: !getUnreadMessagesOnly })

Now, for the case where all we care is to know if we need to render the badge, do we need a page size of 100? Feels like all that data is unused, all we care is to know if there's one or more messages to show the badge but we don't care about the data. We could even send a page_size of 0 and only care about the count in the response right?.

fetchAllMessages(params)
.then((results = []) => {
dispatch(fetchMessagesSuccess({ results }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is old code, but given the ticket and the refactor, shouldn't we take this logic to its own duck? Maybe a messages duck so the app has a single store for them?

})
.catch((error) => console.warn('error fetching messages', { error }));
}, [dispatch]);

useEffect(() => {
fetchMenuMessages();
}, [dispatch, fetchMenuMessages]);
if (subjects.length > 0){
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed because internally fetchMessagesSuccess action is calling messageIsValidForDisplay function which grabs data from subjectStore, so most of the times the store was empty/not-ready and the comparison threw a false negative

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think I understand the logic of this. I feel like this needs to be re-done, since we shouldn't care if the subjects are loaded or not. We just want to fetch the unread messages to show a badge, independently of the subjects that sent the message, why do we need to wait? I guess its a constraint of old logic, but we shouldn't rely on that, if we need to re-work old selectors, reducers, etc... Or simply add new ones, we should!

fetchMenuMessages(true);
}
}, [dispatch, fetchMenuMessages, subjects]);

return canShowMessageMenu ? <Dropdown
align="end"
Expand Down