Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 7 additions & 4 deletions src/Nav/MessageMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import {
fetchAllMessages,
fetchMessagesSuccess,
fetchUnreadMessagesCount,
updateMessageFromRealtime, updateUnreadMessagesCount
updateMessageFromRealtime,
updateUnreadMessagesCount
} from '../ducks/messaging';
import MessageContext from '../InReach/context';

Expand Down Expand Up @@ -45,7 +46,7 @@ const MessageMenu = () => {

const checkForUnreadMessages = useCallback(() => {
fetchUnreadMessagesCount()
.then(({ data: { data: { count } } }) => dispatch(updateUnreadMessagesCount(count)))
.then(({ data: { data: { results } } }) => dispatch(updateUnreadMessagesCount({ results }) ))
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 understand why now we are caring about the results instead of just the count (a number) of unread messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the need of filtering valid messages for the user, having just a count is not enough

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

Expand All @@ -58,8 +59,10 @@ const MessageMenu = () => {
}, [dispatch]);

useEffect(() => {
checkForUnreadMessages();
}, [checkForUnreadMessages, state.results]);
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.

Also, the checkForUnreadMessages is having a race condition with messageIsValidForDisplay in the reducer, so I'm just making sure subjects is available when filtering out valid messages

checkForUnreadMessages();
}
}, [checkForUnreadMessages, state.results, subjects]);

return canShowMessageMenu ? <Dropdown
align="end"
Expand Down
4 changes: 2 additions & 2 deletions src/ducks/messaging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export const updateUnreadMessagesCount = (payload) => ({

const { get, post } = axios;

export const fetchUnreadMessagesCount = () => axios.get(`${MESSAGING_API_URL}?include_additional_data=false&page_size=0&read=false`);
export const fetchUnreadMessagesCount = () => axios.get(`${MESSAGING_API_URL}?include_additional_data=false&page_size=10&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.

I added the page_size to 10 because for the UI starting from 9 it doesn't show the real count of unread messages, it shows a 9+ icon
So just by knowing there are at least more than 10 unread messages it will be enough for the UI to behave as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why is it better to have a page size of 10 instead of 0? Having a page size of 0 made the response extremely small of size 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the messages are needed, they need to be filtered out, if there is an unread message it does not mean that is valid for user to be seen, it needs to be compared against the subject store to check for the messaging prop, check messageIsValidForDisplay function in utils/messaging

Honestly I think this responsibility should be part of the backend calculations, the API should be able to say 'Here are the unread message that concerns only you' but it doesn't, not sure why

cc
@JoshuaVulcan

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok this bit is clear. But then, again, only fetching 10 has a clear bug. I think we need to fetch them all because we don't know if we have 100 messages and just the last 10 are valid for display. If we only fetch the first 10, they will be filtered and show no notification badge. @JoshuaVulcan The fact that we need to filter them in the client doesn't really let us optimize this IMO.


export const fetchMessages = (params = {}) => {
const paramString = objectToParamString(
Expand Down Expand Up @@ -106,7 +106,7 @@ export const messageListReducer = (state = INITIAL_MESSAGE_LIST_STATE, action) =
if (type === UPDATE_UNREAD_MESSAGES_COUNT) {
return {
...state,
unreadMessagesCount: payload,
unreadMessagesCount: payload.results.filter(msg => messageIsValidForDisplay(msg, store.getState().data.subjectStore)).length,
Copy link
Contributor Author

@gaboDev gaboDev Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This filter might be controversial, but due the page_size parameter, this will iterate through it 10 items tops.

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 there's a logical error here. We are just asking for 10 results and then doing this filter, which may filter out only 5 messages, so we would assume there are 5 unread messages. But isn't it possible that in the next page (the next 10 results) we have more unread messages that would pass the filter? That sounds possible. We may filter out some results from the first page but maybe there are more valid unread messages in the next pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it kind of sounds like a design problem, because currently We are being hacky with the GET API of messaging, in that case We should have an endpoint for checking specifically the unread messages count, right?

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 get why a design problem 🤔 I mean, I think this logic of fetching 10 and filtering (that is added in this PR) sounds like has a logical issue: If you have 20 messages in DB and first 10 won't pass this messageIsValidForDisplay filter but the next 10 will, you would save unreadMessagesCount = 0, but you have 10 unread messages in the next page!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correcto, but the whole point of this ticket was to optimize the calculation of unread messages, so now We have to add pagination to it? For me it kind of impact the optimization goal

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but the idea is to optimize it without injecting a bug 😓 What's the benefit for our users if we make it faster but broken? If that is not possible, then I agree with Joshua: let's close this ticket and ask the backend guys for an endpoint update to return only the valid messages.

};
}

Expand Down
Loading