Skip to content

Add pagination info in the query parameter #1417

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

Open
sadiqkhoja wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add pagination info in the query parameter #1417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 12 additions & 6 deletions src/components/entity/table-view.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ except according to the terms contained in the LICENSE file.
type="entity"
:top="pagination.size"
:filter="filter != null || !!searchTerm"
:total-count="dataset.dataExists ? dataset.entities : 0"/>
:total-count="dataset.dataExists && !pagination.page ? dataset.entities : 0"/>
<!-- @update:page is emitted on size change as well -->
<Pagination v-if="pagination.count > 0"
v-model:page="pagination.page" v-model:size="pagination.size"
Expand All @@ -34,6 +34,7 @@ import { computed, inject, reactive, useTemplateRef, watch } from 'vue';
import EntityTable from './table.vue';
import OdataLoadingMessage from '../odata-loading-message.vue';
import Pagination from '../pagination.vue';
import usePaginationQueryRef from '../../composables/pagination-query-ref';

import { apiPaths } from '../../util/request';
import { noop, reemit, reexpose } from '../../util/util';
Expand All @@ -60,9 +61,10 @@ const datasetName = inject('datasetName');
const { dataset, odataEntities, deletedEntityCount } = useRequestData();

const pageSizeOptions = [250, 500, 1000];
const { pageNumber, pageSize } = usePaginationQueryRef(pageSizeOptions);
const pagination = reactive({
page: 0,
size: pageSizeOptions[0],
page: pageNumber,
size: pageSize,
count: computed(() => (odataEntities.dataExists ? odataEntities.count : 0)),
removed: computed(() => (odataEntities.dataExists ? odataEntities.removedEntities : 0))
});
Expand All @@ -71,9 +73,7 @@ const pagination = reactive({
// request. `refresh` indicates whether the request is a background refresh
// (whether the refresh button was pressed).
const fetchChunk = (clear, refresh = false) => {
// Are we fetching the first chunk of entities or the next chunk?
const first = clear || refresh;
if (first) {
if (refresh) {
pagination.page = 0;
}

Expand All @@ -97,6 +97,11 @@ const fetchChunk = (clear, refresh = false) => {
clear
})
.then(() => {
const lastPage = Math.max(0, Math.ceil(odataEntities.count / pagination.size) - 1);
if (pagination.page > lastPage) {
pagination.page = lastPage;
fetchChunk(true);
}
if (props.deleted) {
deletedEntityCount.cancelRequest();
if (!deletedEntityCount.dataExists) {
Expand All @@ -109,6 +114,7 @@ const fetchChunk = (clear, refresh = false) => {
};
fetchChunk(true);
watch([() => props.deleted, () => props.filter, () => props.searchTerm], () => {
pagination.page = 0;
fetchChunk(true);
});
const handlePageChange = () => {
Expand Down
24 changes: 16 additions & 8 deletions src/components/submission/table-view.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ except according to the terms contained in the LICENSE file.
type="submission"
:top="pagination.size"
:filter="!!filter"
:total-count="totalCount"/>
:total-count="pagination.page ? 0 : totalCount"/>
<!-- @update:page is emitted on size change as well -->
<Pagination v-if="pagination.count > 0"
v-model:page="pagination.page" v-model:size="pagination.size"
Expand All @@ -34,6 +34,7 @@ import { computed, reactive, useTemplateRef, watch } from 'vue';
import OdataLoadingMessage from '../odata-loading-message.vue';
import Pagination from '../pagination.vue';
import SubmissionTable from './table.vue';
import usePaginationQueryRef from '../../composables/pagination-query-ref';

import { apiPaths } from '../../util/request';
import { noop, reemit, reexpose } from '../../util/util';
Expand Down Expand Up @@ -73,9 +74,10 @@ const emit = defineEmits(['review', 'delete', 'restore']);
const { odata, deletedSubmissionCount } = useRequestData();

const pageSizeOptions = [250, 500, 1000];
const { pageNumber, pageSize } = usePaginationQueryRef(pageSizeOptions);
const pagination = reactive({
page: 0,
size: pageSizeOptions[0],
page: pageNumber,
size: pageSize,
count: computed(() => (odata.dataExists ? odata.count : 0)),
removed: computed(() => (odata.dataExists ? odata.removedSubmissions : 0))
});
Expand All @@ -88,12 +90,10 @@ const odataSelect = computed(() => {
});

// `clear` indicates whether this.odata should be cleared before sending the
// request. `refresh` indicates whether the request is a background refresh
// request. `refresh` indicates whether the request is a background refresh.
// (whether the refresh button was pressed).
const fetchChunk = (clear, refresh = false) => {
// Are we fetching the first chunk of submissions or the next chunk?
const first = clear || refresh;
if (first) {
if (refresh) {
pagination.page = 0;
Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

I'm trying to think through the ramifications of pagination.page = 0; for a background refresh. Wouldn't that cause the pagination control to immediately change to the first page? But the actual table wouldn't change until the request completes. If that takes a while, I think it'd be confusing for the pagination control to say that it's on the first page. Or even worse, if the request results in an error, the pagination control would incorrectly think that it's on the first page.

If it's a background refresh, I feel like pagination.page should be set to 0 only once the response is received.

It kind of feels like that would be changing the current behavior of background refresh though. Right now, does it refresh the current page rather than returning to the first page? If so, I feel like changing that behavior should maybe go in its own PR. Then this PR could just be focused on query parameters.

}

Expand All @@ -115,6 +115,11 @@ const fetchChunk = (clear, refresh = false) => {
clear
})
.then(() => {
const lastPage = Math.max(0, Math.ceil(odata.count / pagination.size) - 1);
if (pagination.page > lastPage) {
pagination.page = lastPage;
fetchChunk(true);
}
if (props.deleted) {
deletedSubmissionCount.cancelRequest();
if (!deletedSubmissionCount.dataExists) {
Expand All @@ -126,7 +131,10 @@ const fetchChunk = (clear, refresh = false) => {
.catch(noop);
};
fetchChunk(true);
watch([() => props.filter, () => props.deleted], () => { fetchChunk(true); });
watch([() => props.filter, () => props.deleted], () => {
pagination.page = 0;
Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

Right now, if you change the selection of columns / form fields, you're sent back to the first page. But I'm only seeing this line pagination.page = 0; for filter changes, not a change to the column selection. I could be convinced that the current behavior isn't important and is fine to change, but I'm wondering if there's a different way to approach this. I kind of like how fetchChunk() used to be responsible for setting pagination.page in more cases. What if fetchChunk() took a page number argument that defaulted to 0? That would also work for the case where the user navigates past the last page. Like I said, I'm open to various approaches here, I just wanted to throw out these thoughts.

fetchChunk(true);
});
watch(() => props.fields, (_, oldFields) => {
// SubmissionList resets column selector when delete button is pressed, in
// that case we don't want to send request from here.
Expand Down
11 changes: 10 additions & 1 deletion src/composables/data-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,16 @@ export default () => {

const dataView = useQueryRef({
fromQuery: (query) => (!query.deleted && query.map === 'true' ? 'map' : 'table'),
toQuery: (value) => ({ map: value === 'map' ? 'true' : null })
toQuery: (value) => {
if (value === 'map') {
return {
map: true,
'page-size': null,
'page-number': null
};
}
return { map: null };
}
});

const options = computed(() => [
Expand Down
41 changes: 41 additions & 0 deletions src/composables/pagination-query-ref.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
Copyright 2025 ODK Central Developers
See the NOTICE file at the top-level directory of this distribution and at
https://github.com/getodk/central-frontend/blob/master/NOTICE.

This file is part of ODK Central. It is subject to the license terms in
the LICENSE file found in the top-level directory of this distribution and at
https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
including this file, may be copied, modified, propagated, or distributed
except according to the terms contained in the LICENSE file.
*/
Comment on lines +1 to +11
Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

We don't need these file headers for new files anymore. 🥳 So this can be removed.


import useQueryRef from './query-ref';

export default (pageSizeOptions = [250, 500, 1000]) => {
Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

I'm not sure we need a default value here. It's hard for me to think of a case where pageSizeOptions wouldn't be provided. sizeOptions is a required prop in the Pagination component, so it should always exist. If we were to add the concept of a default set of size options, that should probably go in multiple places (e.g., Pagination as well).

const pageNumber = useQueryRef({
fromQuery: (query) => {
const num = Number.parseInt(query['page-number'], 10);
Copy link
Contributor Author

@sadiqkhoja sadiqkhoja Nov 26, 2025

Choose a reason for hiding this comment

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

I using page-number for the readability sake, we can also go with just page.

Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

I definitely prefer page over page-number. It's much shorter, and I see it on many sites. page + page-size sounds like a nice combo to me. I don't have the strongest feelings about it though, we could run with page-number for now and see if anyone else on the team voices an opinion.

if (!num || num < 1) return 0;
return num - 1;
Copy link
Contributor Author

@sadiqkhoja sadiqkhoja Nov 26, 2025

Choose a reason for hiding this comment

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

should I update the query parameter here? if the given value is out of valid range? we don't do that for invalid values of filters, right?

Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

we don't do that for invalid values of filters, right?

That's right. If filter query parameters are supplied invalid values, those values are generally ignored. The URL isn't changed/corrected, but nothing explodes either. The invalid values fall back to something reasonable.

In some cases, we can't know right away whether a value is valid. For example, specifying a submitter ID that doesn't exist: we can't know that it's wrong until we receive the response for the list of submitters (async).

should I update the query parameter here?

I'm open to it, but it feels more complex to me. It seems simpler not to change the route if we don't need to. It should be pretty rare that someone specifies an invalid value.

if the given value is out of valid range?

"Out of valid range" just refers to if (!num || num < 0) return 0; right? That logic sounds great to me.

Copy link
Contributor Author

@sadiqkhoja sadiqkhoja Nov 26, 2025

Choose a reason for hiding this comment

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

I guess page number starting from 1 is more acceptable than 0.

},
toQuery: (value) => ({ 'page-number': value === 0 ? null : value + 1 })
});

const pageSize = useQueryRef({
Copy link
Contributor Author

@sadiqkhoja sadiqkhoja Nov 26, 2025

Choose a reason for hiding this comment

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

Expecting absolute value for page sizes like 250, 500, 1000 instead of index of options here.

fromQuery: (query) => {
const size = Number.parseInt(query['page-size'], 10);

if (!size || size < pageSizeOptions[0]) return pageSizeOptions[0];
if (size >= pageSizeOptions[2]) return pageSizeOptions[2];
if (size >= pageSizeOptions[1]) return pageSizeOptions[1];
return pageSizeOptions[0];
},
toQuery: (value) => ({ 'page-size': value })
});

return {
pageNumber,
pageSize
};
};
69 changes: 67 additions & 2 deletions test/components/entity/list.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,9 +929,74 @@ describe('EntityList', () => {
})
.respondWithSuccess();
const text = component.get('.pagination .form-group').text();

text.should.equal('Rows 1–249 of 259');
});

it('adds page-size query parameter when page size is changed', () => {
createEntities(251);
return load('/projects/1/entity-lists/trees/entities')
.complete()
.request(component => {
const sizeDropdown = component.find('.pagination select:has(option[value="500"])');
return sizeDropdown.setValue(500);
})
.respondWithData(() => testData.entityOData(500))
.afterResponse(component => {
component.vm.$route.query['page-size'].should.equal('500');
});
});

it('adds page-number query parameter when next page is clicked', () => {
createEntities(251);
return load('/projects/1/entity-lists/trees/entities')
.complete()
.request(component =>
component.find('button[aria-label="Next page"]').trigger('click'))
.respondWithData(() => testData.entityOData(250, 250))
.afterResponse(component => {
component.vm.$route.query['page-number'].should.equal('2');
});
});

it('displays the correct page when page-number is provided in URL', () => {
createEntities(501);
return load('/projects/1/entity-lists/trees/entities?page-number=2', { root: false })
.afterResponse(component => {
component.find('.pagination .form-group').text().should.be.equal('Rows 251–500 of 501');
});
});

it('displays correct number of rows when page-size is provided in URL', () => {
createEntities(600);
return load('/projects/1/entity-lists/trees/entities?page-size=500', { root: false })
.afterResponse(component => {
component.find('.pagination select:has(option[value="500"])').element.value.should.be.eql('500');
});
});

it('selects first page when page-number is less than 1 in URL', () => {
createEntities(251);
return load('/projects/1/entity-lists/trees/entities?page-number=0', { root: false })
.afterResponse(component => {
component.find('.pagination .form-group').text().should.be.eql('Rows 1–250 of 251');
});
});

it('selects last page when page-number is greater than last page in URL', () => {
createEntities(501);
return load('/projects/1/entity-lists/trees/entities?page-number=999', { root: false })
.afterResponse(component => {
component.find('.pagination .form-group').text().should.be.eql('Row 501 of 501');
});
});

it('floors page-size to nearest valid value when invalid page-size is provided in URL', () => {
Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

For some of these edge cases, I feel like they'd be better tested in unit tests of the usePaginationQueryRef() composable rather than in every component that uses the composable. There are already so many tests of the EntityList component.

withSetup() provides a way to unit-test a composable. We have unit tests of useQueryRef() that you might find interesting to look at.

Given that these tests are already written, I'm happy to run with them as-is. Mostly I just wanted to share the thought that I think there can be benefit to unit-testing composables and that withSetup() provides a way to do so.

createEntities(251);
return load('/projects/1/entity-lists/trees/entities?page-size=350', { root: false })
.afterResponse(component => {
component.find('.pagination select:has(option[value="500"])').element.value.should.be.eql('250');
});
});
});

describe('deleted entities', () => {
Expand Down Expand Up @@ -962,7 +1027,7 @@ describe('EntityList', () => {

it('updates the deleted count', () => {
testData.extendedEntities.createPast(1, { deletedAt: new Date().toISOString() });
return load('/projects/1/entity-lists/truee/entities', { root: false, container: { router: testRouter() } })
return load('/projects/1/entity-lists/tree/entities', { root: false, container: { router: testRouter() } })
.complete()
.request(component =>
component.get('.toggle-deleted-entities').trigger('click'))
Expand Down
5 changes: 5 additions & 0 deletions test/components/submission/field-dropdown.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import testData from '../../data';
import { loadSubmissionList } from '../../util/submission';
import { mergeMountOptions, mount } from '../../util/lifecycle';
import { testRequestData } from '../../util/request-data';
import { mockLogin } from '../../util/session';

const mountComponent = (options) =>
mount(SubmissionFieldDropdown, mergeMountOptions(options, {
Expand All @@ -25,6 +26,10 @@ const strings = (min, max) => {
};

describe('SubmissionFieldDropdown', () => {
beforeEach(() => {
mockLogin();
Copy link
Member

@matthew-white matthew-white Nov 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why is mockLogin() needed here out of curiosity? Something to do with the router?

Copy link
Contributor Author

@sadiqkhoja sadiqkhoja Nov 26, 2025

Choose a reason for hiding this comment

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

because I changed the router from mock to test in test/util/submission.js, with that change request to sessions is made causing the tests to fail.

Another thing I can do is to add replace method to the list of supported method for mockRouter.

Copy link
Member

@matthew-white matthew-white Mar 15, 2026

Choose a reason for hiding this comment

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

Another thing I can do is to add replace method to the list of supported method for mockRouter.

I'm open to this. It sure would be nice for there to be fewer cases in which testRouter() is needed. As-is, the difference between mockRouter() and testRouter() can feel sort of complicated.

Part of my idea for mockRouter() was that it isolates the individual component being tested from all the logic in the router (the beforeEach guards, etc.). To keep with that theme, maybe replace() could look something like this in mockRouter():

replace: (location) => {
  currentRoute.value = router.resolve(location);
},

In other words, it's not actually doing a navigation (it's not actually calling router.replace()). Instead, it's just setting currentRoute as if the navigation took place. mockRouter() would no longer be read-only, but it would still be different from testRouter() in that it would never run navigation hooks.

Some other things would have to change in mockRouter() though, like the way that $route is provided on app install. It's just a constant right now.

This path sounds promising, but also no need to go down it right now if it doesn't feel like the right time. I'm happy with us using mockLogin() here.

});

it('passes an option for each selectable field', () => {
const component = mountComponent({
container: {
Expand Down
92 changes: 92 additions & 0 deletions test/components/submission/list.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,5 +979,97 @@ describe('SubmissionList', () => {
]);
});
});

it('adds page-size query parameter when page size is changed', () => {
createSubmissions(251);
return load('/projects/1/forms/f/submissions')
.complete()
.request(component => {
const sizeDropdown = component.find('.pagination select:has(option[value="500"])');
return sizeDropdown.setValue(500);
})
.respondWithData(() => testData.submissionOData(500))
.afterResponse(component => {
component.vm.$route.query['page-size'].should.equal('500');
});
});

it('adds page-number query parameter when next page is clicked', () => {
createSubmissions(251);
return load('/projects/1/forms/f/submissions')
.complete()
.request(component =>
component.find('button[aria-label="Next page"]').trigger('click'))
.respondWithData(() => testData.submissionOData(250, 250))
.afterResponse(component => {
component.vm.$route.query['page-number'].should.equal('2');
});
});

it('displays the correct page when page-number is provided in URL', () => {
createSubmissions(501);
return load('/projects/1/forms/f/submissions?page-number=2', { root: false })
.afterResponse(component => {
component.find('.pagination .form-group').text().should.be.equal('Rows 251–500 of 501');
});
});

it('displays correct number of rows when page-size is provided in URL', () => {
createSubmissions(600);
return load('/projects/1/forms/f/submissions?page-size=500', { root: false })
.afterResponse(component => {
component.find('.pagination select').element.value.should.be.eql('500');
});
});

it('selects first page when page-number is less than 1 in URL', () => {
createSubmissions(251);
return load('/projects/1/forms/f/submissions?page-number=0', { root: false })
.afterResponse(component => {
component.find('.pagination .form-group').text().should.be.eql('Rows 1–250 of 251');
});
});

it('selects last page when page-number is greater than last page in URL', () => {
createSubmissions(501);
return load('/projects/1/forms/f/submissions?page-number=999', { root: false })
.afterResponse(component => {
component.find('.pagination .form-group').text().should.be.eql('Row 501 of 501');
});
});

it('floors page-size to nearest valid value when invalid page-size is provided in URL', () => {
createSubmissions(251);
return load('/projects/1/forms/f/submissions?page-size=350', { root: false })
.afterResponse(component => {
component.find('.pagination select').element.value.should.be.eql('250');
});
});

it('removes page-number from query parameter when switching to map view', () => {
const { geopoint } = testData.fields;
const fields = [geopoint('/location')];
testData.extendedForms.createPast(1, { fields });
testData.extendedSubmissions.createPast(251);

return load('/projects/1/forms/f/submissions', { container: { router: testRouter() } })
.complete()
.request(component =>
component.find('button[aria-label="Next page"]').trigger('click'))
.respondWithData(() => testData.submissionOData(250, 250))
.afterResponse(component => {
// Verify we're on page 2
component.vm.$route.query['page-number'].should.equal('2');
})
.request(component => {
const radioField = component.getComponent('.radio-field');
const mapOption = radioField.findAll('input[type="radio"]')[1];
return mapOption.trigger('change');
})
.respondWithData(testData.submissionGeojson)
.afterResponse(component => {
component.vm.$route.query.should.not.have.property('page-number');
});
});
});
});
Loading