-
Notifications
You must be signed in to change notification settings - Fork 438
Bringing deep groups children data to the front-end #1520
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.
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
base: development
Are you sure you want to change the base?
Bringing deep groups children data to the front-end #1520
Conversation
…ty and the new deepGroups property. I also added the deepGroups data fetched to the formatGroupForResponse to be included with GroupData.
…p function. Not required since data is populated in arrays deepGroups and deepMeters. Additionally, added deepGroups with default value under CreateGroupModalComponent.tsx
huss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to @S0nbr4ndonz & @asherdrake for this contribution. Review found it brings in the desired state. I have made a few comments to consider. Please let me know if anything is not clear or you have thoughts. I'm happy to discuss or help with the idea of changing how the state is requested.
…ET route /deep/groups to retrieve deepGroups data and to allow for the data to only be fetched when it is needed. Additionally, also omitted deepMeters and deepGroups data as they are not currently accepted by server schema.
|
Good Morning Steven! I have re-requested a review from you for our fork as I have made a few changes based off of your feedback. First, I have tweaked the root GET route '/' to only return deepMeters to the users and not deepGroups, as I had made it do before. Now, the deep groups data is fetched and returned from the GET route '/deep/groups' and will only currently be used as an endpoint by the groups visual page and only accessible by admins, to decrease payload for all users. Second, I have fixed the issue in /usr/src/app/src/client/app/components/groups/EditGroupModalComponent.tsx by omitting the deepMeters and deeGroups data to fit the server schema. I have checked around the codebase and have not caught any other errors. Third, I believe I have left each file formatted correctly, but please feel free to point out any formatting issues where present! Thank you! |
huss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to @S0nbr4ndonz & @asherdrake for addressing my comments. This now seems fine. I'm not going to approve since I want this held until the visualization page is submitted so there is a use for this new code. It will also be a chance to test it with a real use just to be sure. Please let me know if this is an issue or you have any thoughts.
|
Note the failed tests are not a result of this PR but a general issue that is yet to be resolved. |
Description
@asherdrake and I worked on this change together. This change touches on issue #1509 , where deep groups data must be sent to the front to be used in the solution for issue #1496, group visualization. The changes include an added property deepGroups to the frontend redux state GroupData located at src/client/app/types/redux/groups.ts, tweaking the root GET route '/' to populate the deepGroups property along with the deepMeters property, and including the newly populated deepGroups array into formatGroupForResponse located at src/server/routes/groups.js.
Fixes #1509
Type of change
Checklist
Limitations
None.