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

Show file tree
Hide file tree
Changes from all commits
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
33 changes: 24 additions & 9 deletions src/Nav/MessageMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import { useTranslation } from 'react-i18next';
import { ReactComponent as ChatIcon } from '../common/images/icons/chat-icon.svg';

import { allSubjects } from '../selectors/subjects';
import { fetchAllMessages, fetchMessagesSuccess, updateMessageFromRealtime } from '../ducks/messaging';
import {
fetchAllMessages,
fetchMessagesSuccess,
fetchUnreadMessagesCount,
updateMessageFromRealtime, updateUnreadMessagesCount
} from '../ducks/messaging';
import MessageContext from '../InReach/context';

import Badge from '../Badge';
Expand All @@ -19,6 +24,7 @@ import styles from './styles.module.scss';

const RADIO_MESSAGE_REALTIME = 'radio_message';
const SLEEP_DETECTION_INTERVAL = 60000;
const MENU_MESSAGES_PAGE_SIZE = 100;

const MessageMenu = () => {
const { t } = useTranslation('top-bar', { keyPrefix: 'nav.messageMenu' });
Expand All @@ -29,23 +35,31 @@ const MessageMenu = () => {

const [selectedSubject, setSelectedSubject] = useState(null);

const unreads = state.results.filter((message) => !message.read);
const badgeCount = unreads.length > 9 ? '9+' : unreads.length;
const { unreadMessagesCount } = state;
Copy link
Contributor

@luixlive luixlive Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok so seems like we are not using a Redux reducer but a React reducer. Could we migrate to Redux instead? A React reducer is attached to the lifecycle of the component that holds it (renders), while having to pass down its state and dispatch method (like we are doing with the context). We don't need to reinvent the wheel, and also we could take advantage of having the messages reducer everywhere in the app and even the usage of Redux selectors if we move it to Redux.

Copy link
Contributor

Choose a reason for hiding this comment

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

React reducers are still useful, it doesn't mean that we should have every single reducer in Redux. The main difference is that Redux is designed to be a global store, accessible anywhere in the app, with a defined flow of action -> thunk processing -> reducer -> selectors. Messages data, which comes from DAS and is part of the ER data, should be stored there. React reducers should be used to store component states, just like useState. I know you didn't do it, but I think this ticket is the perfect place to improve old code!

const badgeCount = unreadMessagesCount > 9 ? '9+' : unreadMessagesCount;

const canShowMessageMenu = useMemo(
() => !!state.results.length || !!subjects.filter((subject) => !!subject?.messaging?.length).length,
[state.results.length, subjects]
);

const checkForUnreadMessages = useCallback(() => {
fetchUnreadMessagesCount()
.then(({ data: { data: { count } } }) => dispatch(updateUnreadMessagesCount(count)))
.catch((error) => console.warn('error checking for unread messages', { error }));
}, [dispatch]);

const fetchMenuMessages = useCallback(() => {
fetchAllMessages({ page_size: 100 })
.then((results = []) => dispatch(fetchMessagesSuccess({ results })))
fetchAllMessages({ page_size: MENU_MESSAGES_PAGE_SIZE })
.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]);
checkForUnreadMessages();
}, [checkForUnreadMessages, state.results]);

return canShowMessageMenu ? <Dropdown
align="end"
Expand All @@ -55,11 +69,12 @@ const MessageMenu = () => {
<Dropdown.Toggle aria-label={t('messageMenuToggleLabel')} title={t('messageMenuToggleTitle')}>
<ChatIcon className={styles.messageIcon} />

{!!unreads.length && <Badge className={styles.badge} count={badgeCount} />}
{!!unreadMessagesCount && <Badge className={styles.badge} count={badgeCount} />}
</Dropdown.Toggle>

<Dropdown.Menu>
<MessagesModal onSelectSubject={(subject) => setSelectedSubject(subject)} selectedSubject={selectedSubject} />
<MessagesModal onSelectSubject={(subject) => setSelectedSubject(subject)}
selectedSubject={selectedSubject} />
</Dropdown.Menu>

<StateManagedSocketConsumer
Expand Down
17 changes: 16 additions & 1 deletion src/ducks/messaging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { objectToParamString, recursivePaginatedQuery } from '../utils/query';
import { messageIsValidForDisplay } from '../utils/messaging';

const FETCH_MESSAGES_SUCCESS = 'FETCH_MESSAGES_SUCCESS';
const UPDATE_UNREAD_MESSAGES_COUNT = 'UPDATE_UNREAD_MESSAGES_COUNT';
const REMOVE_MESSAGE = 'REMOVE_MESSAGE';
const SOCKET_MESSAGE_UPDATE = 'SOCKET_MESSAGE_UPDATE';

Expand All @@ -22,7 +23,6 @@ export const fetchMessagesSuccess = (payload, refresh = false) => ({
refresh,
});


export const updateMessageFromRealtime = payload => ({
type: SOCKET_MESSAGE_UPDATE,
payload,
Expand All @@ -33,8 +33,15 @@ export const removeMessageById = id => ({
payload: id,
});

export const updateUnreadMessagesCount = (payload) => ({
type: UPDATE_UNREAD_MESSAGES_COUNT,
payload,
});

const { get, post } = axios;

export const fetchUnreadMessagesCount = () => axios.get(`${MESSAGING_API_URL}?include_additional_data=false&page_size=0&read=false`);

export const fetchMessages = (params = {}) => {
const paramString = objectToParamString(
{ include_additional_data: false, page_size: 25, ...params },
Expand Down Expand Up @@ -67,6 +74,7 @@ export const INITIAL_MESSAGE_LIST_STATE = {
next: null,
previous: null,
count: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's out of scope, but it would be nice to refactor this reducer a bit. It's just storing an Axios response object, instead of a more ER usable data structure of our data. That's why we end up needing complicated selectors that are triggered several times. If we processed the data in our actions and reducers to store them in a more usable way we would need less and simpler selectors. Even the new property you added stands out because it doesn't fit the response object properties haha. If it's not that complicated to do so, it would be a great addition to this PR to rework this a bit!

unreadMessagesCount: 0,
};
export const messageListReducer = (state = INITIAL_MESSAGE_LIST_STATE, action) => {
const { refresh, type, payload } = action;
Expand Down Expand Up @@ -95,6 +103,13 @@ export const messageListReducer = (state = INITIAL_MESSAGE_LIST_STATE, action) =
};
}

if (type === UPDATE_UNREAD_MESSAGES_COUNT) {
return {
...state,
unreadMessagesCount: payload,
};
}

if (type === REMOVE_MESSAGE) {
return {
...state,
Expand Down