Skip to content

ERA-9761: Add page_size to event feed requests #1123

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

Closed
wants to merge 3 commits into from
Closed

ERA-9761: Add page_size to event feed requests #1123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f60754
Add page_size to event feed requests
luixlive May 22, 2024
f307b12
Add include_updates=false to the feed fetch and load every single rep…
luixlive May 24, 2024
0bf3a01
ERA-9761: Add more query params narrow down the response size in the …
luixlive May 30, 2024
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
18 changes: 13 additions & 5 deletions src/ReportManager/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,20 @@ import useNavigate from '../hooks/useNavigate';
import { uuid } from '../utils/string';

import DelayedUnmount from '../DelayedUnmount';
import LoadingOverlay from '../LoadingOverlay';
import ReportDetailView from './ReportDetailView';

import styles from './styles.module.scss';

const ADDED_REPORT_TRANSITION_EFFECT_TIME = 600;

const shouldFetchEventDetails = (eventId, eventStore) =>
!eventStore[eventId]
|| !eventStore[eventId].event_details
|| !eventStore[eventId].files
|| !eventStore[eventId].notes
|| !eventStore[eventId].updates;

const ReportManager = ({ onReportBeingAdded }) => {
const dispatch = useDispatch();
const location = useLocation();
Expand Down Expand Up @@ -90,10 +98,10 @@ const ReportManager = ({ onReportBeingAdded }) => {
}, [onCancelAddedReport]);

useEffect(() => {
if (isNewReport || eventStore[reportId]) {
if (isNewReport) {
setIsLoadingReport(false);
}
}, [eventStore, isNewReport, reportId]);
}, [isNewReport]);

useEffect(() => {
if (isNewReport) {
Expand All @@ -109,7 +117,7 @@ const ReportManager = ({ onReportBeingAdded }) => {
}, [isNewReport, location.pathname, location.search, location.state, navigate, newReportTemporalId, reportType]);

useEffect(() => {
if (!isNewReport && !eventStore[reportId]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer check if the event is in the store, cause probably it is if it was loaded by the feed, but it won't have the updates property.

Copy link
Collaborator

@JoshuaVulcan JoshuaVulcan May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't mind this, but it's a little blunt and takes away an (overall helpful) optimization. Clicking any previously-viewed event will cause duplicate requests. Could we eval with a function to test if the event's data structure is complete and only fetch if not?

if (!isNewReport && shouldFetchEventDetails(reportId, eventStore)) {
setIsLoadingReport(true);
dispatch(fetchEvent(reportId))
.then(() => setIsLoadingReport(false))
Expand All @@ -118,15 +126,15 @@ const ReportManager = ({ onReportBeingAdded }) => {
}, [dispatch, eventStore, isNewReport, navigate, reportId]);

return <TrackerContext.Provider value={reportTracker}>
{shouldRenderReportDetailView && <ReportDetailView
{shouldRenderReportDetailView ? <ReportDetailView
formProps={navigationData?.formProps}
isNewReport={isNewReport}
key={reportId} // This resets component state when the id changes
newReportTypeId={newReportTypeId}
onAddReport={onAddReport}
reportData={reportData}
reportId={reportId}
/>}
/> : <LoadingOverlay />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a loading spinner


<DelayedUnmount isMounted={showAddedReport}>
<ReportDetailView
Expand Down
21 changes: 19 additions & 2 deletions src/ReportManager/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ describe('ReportManager', () => {
});
});

test('fetches the event data if there is an id specified in the URL', async () => {
test('fetches the event data if there is an id specified in the URL and the event is not in the store', async () => {
useLocationMock = jest.fn((() => ({ pathname: '/events/123' })));
useLocation.mockImplementation(useLocationMock);

Expand All @@ -138,6 +138,23 @@ describe('ReportManager', () => {
});
});

test('fetches the event data if there is an id specified in the URL and the event is in the store but is missing properties', async () => {
useLocationMock = jest.fn((() => ({ pathname: '/events/123' })));
useLocation.mockImplementation(useLocationMock);

store.data.eventStore = {
123: {
...eventWithPoint,
updates: undefined,
},
};
renderReportManager(store);

await waitFor(() => {
expect(capturedRequestURLs.find((item) => item.includes(`${EVENT_API_URL}123`))).toBeDefined();
});
});

test('does not fetch the event data if the id is "new"', async () => {
useLocationMock = jest.fn((() => ({ pathname: '/events/new' })));
useLocation.mockImplementation(useLocationMock);
Expand All @@ -150,7 +167,7 @@ describe('ReportManager', () => {
});
});

test('does not fetch the event data if it is in the event store already', async () => {
test('does not fetch the event data if it is in the event store already and complete', async () => {
useLocationMock = jest.fn((() => ({ pathname: '/events/123' })));
useLocation.mockImplementation(useLocationMock);

Expand Down
23 changes: 18 additions & 5 deletions src/SideBar/useReportsFeed/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,17 @@ const useReportsFeed = () => {

return isEqual(restEventFilter, INITIAL_FILTER_STATE);
}, [eventFilter]);
const eventParams = useRef(calcEventFilterForRequest(
{ params: { exclude_contained: shouldExcludeContained }, format: 'object' },
feedSort
));
const eventParams = useRef(calcEventFilterForRequest({
params: {
exclude_contained: shouldExcludeContained,
include_details: false,
include_files: false,
include_notes: false,
include_updates: false,
page_size: 25,
},
format: 'object',
}, feedSort));

const geoResrictedUserLocationCoords = useMemo(
() => userIsGeoPermRestricted && userLocationCoords,
Expand Down Expand Up @@ -61,7 +68,13 @@ const useReportsFeed = () => {
}, [geoResrictedUserLocationCoords, loadFeedEvents]);

useEffect(() => {
const params = {};
const params = {
include_details: false,
include_files: false,
include_notes: false,
include_updates: false,
page_size: 25,
};
if (shouldExcludeContained) {
params.exclude_contained = true;
}
Expand Down