Skip to content
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

[Hotfix] Fix current org change #2402

Merged
merged 5 commits into from
Jan 24, 2025
Merged

[Hotfix] Fix current org change #2402

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28a29c3
fix current org change
Codebmk Jan 24, 2025
a4f28e9
lowercase
Codebmk Jan 24, 2025
21eabe0
id consistency
Codebmk Jan 24, 2025
13c4d11
remove unncessaring loading state update
Codebmk Jan 24, 2025
4d78bb3
remove duplicate org name matching logic
Codebmk Jan 24, 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
2 changes: 1 addition & 1 deletion platform/src/common/components/AQNumberCard/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ const AQNumberCard = ({ className = '' }) => {
[dispatch],
);

if (loading) {
if (loading || isFetchingActiveGroup) {
return (
<div
className={`grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-4 gap-4 ${className}`}
Expand Down
15 changes: 7 additions & 8 deletions platform/src/common/components/Dropdowns/OrganizationDropdown.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,12 @@ const OrganizationDropdown = () => {
setLoading(true);
setSelectedGroupId(group._id);
try {
const response = await dispatch(
await dispatch(
updateUserPreferences({
user_id: userID,
group_id: group._id,
}),
);
if (response?.payload?.success) {
localStorage.setItem('activeGroup', JSON.stringify(group));
dispatch(setOrganizationName(group.grp_title));
} else {
console.warn('Failed to update user preferences');
}
} catch (error) {
console.error('Error updating user preferences:', error);
} finally {
Expand All @@ -84,10 +78,15 @@ const OrganizationDropdown = () => {
const handleDropdownSelect = useCallback(
(group) => {
if (group?._id !== activeGroupId) {
// Immediately update organization name
dispatch(setOrganizationName(group.grp_title));
localStorage.setItem('activeGroup', JSON.stringify(group));

// Dispatch preferences update asynchronously
Comment on lines +81 to +85
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Synchronizing local and remote states.
Immediately updating local storage and Redux could lead to mismatches if the update request fails. Consider deferring local storage updates until the dispatch call succeeds, or handle failures more robustly.

handleUpdatePreferences(group);
}
},
[activeGroupId, handleUpdatePreferences],
[activeGroupId, handleUpdatePreferences, dispatch],
);

if (!activeGroupId || groupList.length === 0) {
Expand Down
9 changes: 7 additions & 2 deletions platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const DataDownload = ({ onClose }) => {
id: activeGroupId,
title: groupTitle,
groupList,
loading: isFetchingActiveGroup,
} = useGetActiveGroup();
const preferencesData = useSelector(
(state) => state.defaults.individual_preferences,
Expand Down Expand Up @@ -128,12 +129,16 @@ const DataDownload = ({ onClose }) => {
* Fetch sites summary whenever the selected organization changes.
*/
useEffect(() => {
if (isFetchingActiveGroup) return;

if (formData.organization) {
dispatch(
fetchSitesSummary({ group: formData.organization.name.toLowerCase() }),
fetchSitesSummary({
group: formData.organization.name.toLowerCase(),
}),
);
}
}, [dispatch, formData.organization]);
}, [dispatch, formData.organization, isFetchingActiveGroup]);

/**
* Clears all selected sites and resets form data.
Expand Down
35 changes: 21 additions & 14 deletions platform/src/core/hooks/useGetActiveGroupId.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,46 @@
import { useEffect, useMemo, useState } from 'react';
import { useEffect, useState } from 'react';
import { useSelector } from 'react-redux';

export function useGetActiveGroup() {
const [activeGroup, setActiveGroup] = useState(null);
const [loading, setLoading] = useState(true);
const userInfo = useSelector((state) => state?.login?.userInfo);
const chartData = useSelector((state) => state.chart);

const activeGroupFromStorage = useMemo(() => {
try {
return JSON.parse(localStorage.getItem('activeGroup') || 'null');
} catch (error) {
console.error('Error parsing activeGroup from local storage:', error);
return null;
}
}, []);
useEffect(() => {
setLoading(false);
}, [userInfo]);

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential race condition in loading state management.

The first useEffect blindly sets loading to false whenever userInfo changes, which could interfere with the loading state managed by the second useEffect. Consider removing this useEffect and handling loading state only in the group matching logic.

-  useEffect(() => {
-    setLoading(false);
-  }, [userInfo]);

useEffect(() => {
setLoading(true);

const matchingGroup = userInfo?.groups?.find(
(group) =>
group.grp_title.toLowerCase() ===
chartData?.organizationName.toLowerCase(),
);

setActiveGroup(matchingGroup);
setLoading(false);
}, [userInfo, activeGroupFromStorage]);
}, [chartData?.organizationName]);

// If no userInfo or groups, return stored or default values
if (!userInfo || !userInfo.groups || !chartData?.organizationName) {
return {
loading,
id: activeGroupFromStorage?.id || null,
title: activeGroupFromStorage?.grp_title || null,
id: activeGroup?.id || null,
title: activeGroup?.grp_title || null,
userID: userInfo?.id || null,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent ID property access across the hook.

The hook uses both _id and id properties when returning group data. This inconsistency could lead to bugs.

- id: activeGroup?.id || null,
+ id: activeGroup?._id || null,

Also, consider consolidating the return object creation into a helper function to ensure consistency:

const createReturnObject = (group, loading, userInfo) => ({
  loading,
  id: group?._id || null,
  title: group?.grp_title || null,
  userID: userInfo?._id || null,
  groupList: userInfo?.groups || [],
});

Also applies to: 49-52

groupList: userInfo?.groups || [],
};
}

// Prioritize stored group if it exists in user's groups
if (activeGroupFromStorage) {
if (chartData.organizationName) {
const storedGroupInUserGroups = userInfo.groups.find(
(group) => group._id === activeGroupFromStorage._id,
(group) =>
group.grp_title.toLowerCase() ===
chartData.organizationName.toLowerCase(),
);

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Duplicate group matching logic detected.

The case-insensitive group matching logic is duplicated. Consider extracting it into a helper function.

+ const findGroupByOrgName = (groups, orgName) => 
+   groups?.find(group => 
+     group.grp_title.toLowerCase() === orgName?.toLowerCase()
+   );

- const storedGroupInUserGroups = userInfo.groups.find(
-   (group) =>
-     group.grp_title.toLowerCase() ===
-     chartData.organizationName.toLowerCase(),
- );
+ const storedGroupInUserGroups = findGroupByOrgName(
+   userInfo.groups, 
+   chartData.organizationName
+ );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (chartData.organizationName) {
const storedGroupInUserGroups = userInfo.groups.find(
(group) => group._id === activeGroupFromStorage._id,
(group) =>
group.grp_title.toLowerCase() ===
chartData.organizationName.toLowerCase(),
);
const findGroupByOrgName = (groups, orgName) =>
groups?.find(group =>
group.grp_title.toLowerCase() === orgName?.toLowerCase()
);
if (chartData.organizationName) {
const storedGroupInUserGroups = findGroupByOrgName(
userInfo.groups,
chartData.organizationName
);

if (storedGroupInUserGroups) {
Expand Down
Loading