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.

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

EES-5542 Maps fetching/rendering boundaries for datasets #5423

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

bennettstuart
Copy link
Collaborator

@bennettstuart bennettstuart commented Nov 28, 2024

Setting map charts to fetch location geo JSON for data sets with boundary levels set.

This has been setup by:

  • drilling releaseId & dataBlockParentId props into MapBlock
  • setting up a useQuery to handle fetching location geo JSON based on data set input selection
    • useQuery cache is initialised with existing subject meta location data to avoid duplicate calls
    • geo JSON location cache is wholly handled by useQuery as it stands and is set to not refetch on alt-tab or stale data, as the response is extremely unlikely to change
  • Added testing to check mapBlock requests new geoJson, unable to test that returned boundary data is renderred due to EES-4902, also created a ticket to rectify this EES-5718
  • Removed feature flag, now surrounded work is completed and merged

@bennettstuart bennettstuart marked this pull request as ready for review December 4, 2024 15:34
@bennettstuart bennettstuart requested review from amyb-hiveit and ntsim and removed request for amyb-hiveit December 4, 2024 15:35
Comment on lines 139 to 169
const queryClient = useQueryClient();
// add existing geoJson to cache, avoiding double fetching
queryClient.setQueryData(
['mapLocationGeoJson', releaseId, dataBlockParentId, boundaryLevel],
meta.locations,
);

const { data: locations, isFetching: isFetchingGeoJson } = useQuery<
LocationFilter[]
>({
queryKey: [
'mapLocationGeoJson',
releaseId,
dataBlockParentId,
selectedBoundaryLevel,
],
queryFn: async () => {
return mapFullTableMetaLocations(
await tableBuilderService.getLocationGeoJson(
releaseId,
dataBlockParentId,
selectedBoundaryLevel,
),
);
},
staleTime: Infinity,
cacheTime: Infinity,
keepPreviousData: true,
refetchOnWindowFocus: false,
refetchOnMount: false,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refactor into tableBuilderQueries

As discussed, we should refactor the GeoJSON fetching into a new getLocationGeoJson query key in tableBuilderQueries.

As part of this, we'll also need to refactor the current implementation of tableBuilderQueries.getDataBlockTable so that the table data is fetched independently of the GeoJSON so that these become two separate queries.

It might also be a good idea to keep the mapFullTable and mapFullTableMetaLocations inside the respective query functions so that we don't need to call them again, but will leave that with you.

Restructuring the data fetching

As we also discussed, it's probably cleaner to hoist the GeoJSON fetching into the parent component and use a callback to pass the boundary level back up whenever it changes.

I'll leave it to you to figure out the details, but it might be a little bit more involved in the chart builder. Lemme know if there's any complications.

The main caveat I can see is around the loading state when the boundary level is being changed. It might be nice if we can avoid having to pass this into MapBlock (from the parent) and control this from within MapBlock itself. An async callback might help with this i.e.

interface MapBlockProps {
  // ...
  onBoundaryLevelChange: (boundaryLevel: number) => Promise<void>;
}

// When boundary level is being changed
setBoundaryLevelChanging(true);

await onBoundaryLevelChange(boundaryLevel);

setBoundaryLevelChanging(false);

expect(loadingSpinner).toHaveTextContent(
'fetching geometry for data selection',
);
expect(tableBuilderService.getLocationGeoJson).toHaveBeenCalled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should assert what it's been called with

test('selecting data set with different boundary level fetches different boundary geo-JSON', async () => {
tableBuilderService.getLocationGeoJson.mockResolvedValueOnce(
testMapTableData.subjectMeta.locations,
); // EES-5718 need to return different location geoJson for
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment seems to just trail off? Not sure what it's trying to say

@bennettstuart bennettstuart force-pushed the EES-5542 branch 3 times, most recently from 3122129 to c0c6c08 Compare December 18, 2024 16:42
@bennettstuart bennettstuart requested a review from ntsim December 19, 2024 14:14
@bennettstuart bennettstuart force-pushed the EES-5542 branch 4 times, most recently from 7ecb527 to 5db0cc7 Compare December 20, 2024 14:55
@bennettstuart bennettstuart force-pushed the EES-5542 branch 2 times, most recently from 1594334 to 8255bad Compare December 20, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants