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

gaboDev · 2025-03-31T21:42:22Z

What does this PR do?

It filters unread messages against messageIsValidForDisplay to take in count active/inactive subjects.
Adds the subject store as dependency for checking unread messages

Relevant link(s)

Tracking tickets: ERA-10594
Hosted demo environments

gaboDev · 2025-03-31T21:44:20Z

src/ducks/messaging.js

@@ -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`);


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.

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 🤔

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

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.

gaboDev · 2025-03-31T21:44:57Z

src/ducks/messaging.js

@@ -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,


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

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.

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?

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!

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

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.

gaboDev · 2025-03-31T21:50:39Z

src/Nav/MessageMenu.js

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

  useEffect(() => {
-    checkForUnreadMessages();
-  }, [checkForUnreadMessages, state.results]);
+    if (subjects.length > 0){


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

luixlive · 2025-03-31T22:23:04Z

src/ducks/messaging.js

@@ -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`);


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 🤔

luixlive · 2025-03-31T22:23:45Z

src/Nav/MessageMenu.js

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

  const checkForUnreadMessages = useCallback(() => {
    fetchUnreadMessagesCount()
-      .then(({ data: { data: { count } } }) => dispatch(updateUnreadMessagesCount(count)))
+      .then(({ data: { data: { results } } }) => dispatch(updateUnreadMessagesCount({ results }) ))


Hmm I don't understand why now we are caring about the results instead of just the count (a number) of unread messages.

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

luixlive · 2025-03-31T22:26:02Z

src/ducks/messaging.js

@@ -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,


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.

JoshuaVulcan · 2025-04-01T00:24:30Z

TBH, I think we should close this and fix the issue properly on the back end...

gaboDev · 2025-04-01T17:04:29Z

We are going to fix this issue in the backend as suggested here:
https://allenai.atlassian.net/browse/ERA-10594?focusedCommentId=125782

Adding validation of messageIsValidForDisplay for unread messages count

9cf468a

gaboDev added the deploy label Mar 31, 2025

gaboDev requested review from AlanCalvillo, luixlive and JoshuaVulcan March 31, 2025 21:42

gaboDev temporarily deployed to ERA-10594 March 31, 2025 21:42 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 31, 2025 21:42 — with GitHub Actions Inactive

gaboDev commented Mar 31, 2025

View reviewed changes

luixlive reviewed Mar 31, 2025

View reviewed changes

gaboDev requested a review from luixlive March 31, 2025 22:51

gaboDev closed this Apr 1, 2025

gaboDev reopened this Apr 1, 2025

gaboDev deployed to ERA-10594 April 1, 2025 17:03 — with GitHub Actions View deployment

gaboDev had a problem deploying to padas-app April 1, 2025 17:04 — with GitHub Actions Failure

gaboDev closed this Apr 1, 2025

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

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

Uh oh!

Conversation

gaboDev commented Mar 31, 2025 • edited by jira bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Relevant link(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaboDev Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan commented Apr 1, 2025

Uh oh!

gaboDev commented Apr 1, 2025

Uh oh!

Uh oh!

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

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

gaboDev commented Mar 31, 2025 •

edited by jira bot

Loading

gaboDev Mar 31, 2025 •

edited

Loading