[To Main] DESENG-643: Final changes for engagement loader optimizations #2558
Conversation
There was a problem hiding this comment.
Thanks for all the great work @jareth-whitney ! There may be some opportunities to optimize code in a few places. However, all that I'll ask to change for sure is the commented-out code. Could you please review instances of it and determine if that code is needed for later or should be removed. If it's the former, then we typically add an accompanying comment to let folks know it's needed.
As mentioned, we're not actually merging this PR and Nat will be continuing the work on this. Instead of making any code changes, you could just signal to her what needs to change, if anything, and she can do it later in the course of her work
| const { metadata } = !isCreate | ||
| ? (useRouteLoaderData('single-engagement') as { metadata: Promise<EngagementMetadata[]> }) | ||
| : noRouteLoaders; | ||
| const { taxa } = !isCreate |
There was a problem hiding this comment.
@jareth-whitney You may be able to reduce a lot of the code by doing something like:
| const { taxa } = !isCreate | |
| let engagement: Promise<Engagement[]>, content<EngagementContent[]>, metadata<EngagementMetadata[]>, taxa<MetadataTaxon[]>; | |
| if (isCreate) { | |
| engagement = useRouteLoaderData('single-engagement'); | |
| content = useRouteLoaderData('single-engagement'); | |
| //... etc. | |
| } | |
There was a problem hiding this comment.
It needs to be destructured due to the way the loader is structured, outputting different values as object properties. Of course, this could be changed.
| if (!isCreate && isNaN(Number(engagementId))) { | ||
| navigate('/'); | ||
| } | ||
|
|
||
| if (isCreate) { | ||
| if (isCreate && !engagementId) { |
There was a problem hiding this comment.
Maybe I'm misreading this but wouldn't the existence of isCreate imply that there's no loaded engagement ID?
There was a problem hiding this comment.
There is a weird third state for the engagement form (besides create and edit) where the creation form is filled out and the user presses "Save and Continue". The create page is essentially functioning as an edit page. I mentioned this in the new ticket, and that users should just be directly redirected to the edit page when hitting "save and continue".
There was a problem hiding this comment.
Oh okay I think I see where the weirdness is - because we need to make an early save? And to the user it still appears as if they're "creating" the engagement for the first time.
It sounds like this is work for the new ticket you were mentioning but in my mind, if the engagement is created, and the form the user uses is functionally identical to the edit form, I feel like we should set isCreate to false. Or maybe rename it to something that better illustrates the particular way we create new engagements in the app.
| setIsNewEngagement(!savedEngagement.id); | ||
| setSavedBannerImageFileName(engagement.banner_filename); | ||
|
|
||
| if (bannerImage) setBannerImage(null); |
There was a problem hiding this comment.
Could you provide a bit of context for this line? If there's a bannerImage loaded then unset it?
There was a problem hiding this comment.
This was existing code, I'm not sure why it's showing up as a new edit. Possibly due to restructuring of the file?
| @@ -52,7 +52,7 @@ export const ContentTabs: React.FC = () => { | |||
| return; | |||
| } | |||
| await deleteEngagementContent(savedEngagement.id, tab_id); | |||
| await fetchEngagementContents(); | |||
| // await fetchEngagementContents(); | |||
There was a problem hiding this comment.
Is this commented-out code needed for later?
There was a problem hiding this comment.
No it can be removed, I missed it.
| @@ -31,8 +30,8 @@ export const EngagementContentContext = createContext<EngagementContentProps>({ | |||
| }, | |||
| }); | |||
|
|
|||
| const CREATE = 'create'; | |||
There was a problem hiding this comment.
I know that setting this constant replicates a pattern from ActionContext but I feel like there isn't much value in doing so if the variable is only used once.
There was a problem hiding this comment.
Yes I was just following the previous pattern so this could be used as a string value when it's actually needed.
| key: 'name', | ||
| value: '', | ||
| }); | ||
| const [searchText, setSearchText] = useState(''); | ||
| const [engagements, setEngagements] = useState<Engagement[]>([]); | ||
| const { engagements } = useRouteLoaderData('engagement-listing') as { engagements: Engagement[] }; |
There was a problem hiding this comment.
Is there a reason you need to type cast the return value as an object and then destructure engagements? Would you be able to do:
| const { engagements } = useRouteLoaderData('engagement-listing') as { engagements: Engagement[] }; | |
| const engagements = useRouteLoaderData('engagement-listing') as Engagement[]; |
There was a problem hiding this comment.
Due to the nature of the loader component, everything is assigned a property, thus the destructuring here. Of course, this could be changed.
| setPaginationOptions(paginationOptions) | ||
| } | ||
| paginationOptions={paginationOptions} | ||
| // loading={tableLoading} |
There was a problem hiding this comment.
Does this commented code need to be here?
There was a problem hiding this comment.
I think this could be revisited. I believe I disabled it because it was highjacking the page (infinite loading) until I disabled it, but it could be reintegrated.
| const sort_key = searchParams.get('sort_key'); | ||
| const sort_order = searchParams.get('sort_order'); | ||
| const search_text = searchParams.get('search_text'); | ||
| const engagement_status = searchParams.get('engagement_status')?.split('_'); |
There was a problem hiding this comment.
Can searchParams.get('engagement_status') ever be an empty string? If so, Number(status) on line 21 below will output 0. Just want to ensure that that's the expected behaviour
There was a problem hiding this comment.
I can't speak to the specific output of engagement_status but the search filter was tested extensively and no issues came up. I would have to take a look at the code again to know for sure.
There was a problem hiding this comment.
No need to dive into it! Thanks for reassuring me
| const contentSummary = content.then((response) => | ||
| Promise.all( | ||
| response.map((content) => { | ||
| return content.content_type === 'Summary' | ||
| ? getSummaryContent(content.id) | ||
| ? getSummaryContent(Number(content.id)).then(convertArrayToSingleEntry) |
There was a problem hiding this comment.
Is convertArrayToSingleEntry needed?
| ? getSummaryContent(Number(content.id)).then(convertArrayToSingleEntry) | |
| ? getSummaryContent(Number(content.id)).then(engagementSummaryContent => engagementSummaryContent[0]) |
There was a problem hiding this comment.
Yes it breaks when it's removed. Of course, this could be an inline function as well if you think this is too verbose.
| @@ -233,12 +206,11 @@ export const ActionProvider = ({ children }: { children: JSX.Element | JSX.Eleme | |||
| }, [slug]); | |||
|
|
|||
| useEffect(() => { | |||
| fetchEngagement(); | |||
| // revalidator.revalidate(); | |||
There was a problem hiding this comment.
Some more commented out code in this file - necessary going forward?
There was a problem hiding this comment.
Nope, it can be removed.
| <Route path=":engagementId/comments/:dashboardType" element={<EngagementComments />} /> | ||
| <Route path=":slug/dashboard/:dashboardType" element={<PublicDashboard />} /> | ||
| <Route path=":engagementId/dashboard/:dashboardType" element={<PublicDashboard />} /> | ||
| <Route path=":slug/comments/:dashboardType" element={<EngagementComments />} /> |
There was a problem hiding this comment.
This route structure could be improved a bit - anything with an :engagementId segment can be nested under the :engagmentId route after deleting that prefix. The :slug paths could be re-nested as well
There was a problem hiding this comment.
Yes, good points Nat, I missed that.
| @@ -12,7 +12,7 @@ import { RedirectLogin } from './RedirectLogin'; | |||
| import withLanguageParam from './LanguageParam'; | |||
| import { Navigate, Route } from 'react-router-dom'; | |||
| import NotFound from './NotFound'; | |||
| import ViewEngagement, { engagementLoader } from 'components/engagement/new/view'; | |||
| import ViewEngagement, { engagementLoader } from 'components/engagement/new/view'; // TODO: Convert tenant calls to route loader | |||
| import { SurveyLoader } from 'components/survey/building/SurveyLoader'; | |||
There was a problem hiding this comment.
Does this mean that tenant fetching should be deferred to the loader? Tenant info is already stored in the authentication slice
There was a problem hiding this comment.
This looks to be an existing comment, not something from me. It's possible that you removed it from the main branch when you uploaded your tenant loader commit, but it was still present on my instance, and thus you're seeing it here.
| paginationOptions?.page && url.searchParams.set('page', paginationOptions.page.toString()); | ||
| paginationOptions?.size && url.searchParams.set('size', paginationOptions.size.toString()); | ||
| paginationOptions?.sort_key && | ||
| url.searchParams.set('sort_key', 'engagement.' + paginationOptions.sort_key.toString()); |
There was a problem hiding this comment.
nested_sort_key could probably be used here instead since it includes the model. prefix
There was a problem hiding this comment.
Hm ok, I tried printing out the nested sort key value at first and it seems to function differently from the sort_key. Is this not a secondary sort?
…ting-loader-improvements [To Feature] Improve loader features on authenticated engagement search page
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2558 +/- ##
==========================================
+ Coverage 75.96% 75.99% +0.03%
==========================================
Files 609 609
Lines 21960 21924 -36
Branches 1711 1747 +36
==========================================
- Hits 16682 16662 -20
+ Misses 5015 4998 -17
- Partials 263 264 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…-creation-flow [To Feature] DESENG 664: Fix engagement creation flow
[To Feature] Update unit tests for DESENG-664
|
NatSquared
changed the title
Issue #: 🎟️ DESENG-643
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).