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

Fix pagination after increasing items per page #8854

Merged
merged 2 commits into from
Apr 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Bugfix: Pagination after increasing items per page

An issue where the file list incorrectly showed no items after paginating and increasing the amount of items per page has been fixed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An issue where the file list incorrectly showed no items after paginating and increasing the amount of items per page has been fixed.
An issue where the file list incorrectly showed no items after paginating and changing the amount of items per page has been fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was about to do the same, but the bug report is still right though, no? The issue only occurred when increasing the amount of items.

Copy link
Member

Choose a reason for hiding this comment

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

yes :D fine by me either way


https://github.com/owncloud/web/issues/6768
https://github.com/owncloud/web/pull/8854
58 changes: 41 additions & 17 deletions packages/web-app-files/src/components/AppBar/ViewOptions.vue
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
v-if="viewModes.includes(ViewModeConstants.tilesView)"
class="files-view-options-list-item oc-visible@s oc-flex oc-flex-between oc-flex-middle"
>
<label for="tiles-size-slider" v-text="resizeTilesLabel" />
<label for="tiles-size-slider" v-text="$gettext('Tile size')" />
<input
v-model="viewSizeCurrent"
type="range"
Expand All @@ -89,9 +89,16 @@
</template>

<script lang="ts">
import { defineComponent, PropType, ref, unref, watch } from 'vue'
import { computed, defineComponent, PropType, ref, unref, watch } from 'vue'
import { mapMutations, mapState } from 'vuex'
import { queryItemAsString, useRouteQueryPersisted } from 'web-pkg/src/composables'
import { useGettext } from 'vue3-gettext'
import {
queryItemAsString,
useRoute,
useRouteQuery,
useRouteQueryPersisted,
useRouter
} from 'web-pkg/src/composables'
import { ViewMode } from 'web-pkg/src/ui/types'
import { PaginationConstants, ViewModeConstants } from '../../composables'

Expand All @@ -106,8 +113,19 @@ export default defineComponent({
}
},
setup() {
const router = useRouter()
const currentRoute = useRoute()
const { $gettext } = useGettext()

const queryParamsLoading = ref(false)

const currentPageQuery = useRouteQuery('page')
const currentPage = computed(() => {
if (!unref(currentPageQuery)) {
return 1
}
return parseInt(queryItemAsString(unref(currentPageQuery)))
})
const itemsPerPageQuery = useRouteQueryPersisted({
name: PaginationConstants.perPageQueryName,
defaultValue: PaginationConstants.perPageDefault
Expand All @@ -130,6 +148,21 @@ export default defineComponent({
}
}

const setItemsPerPage = (itemsPerPage: string) => {
const resetPagination = itemsPerPage > unref(itemsPerPageQuery) && unref(currentPage) > 1
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should go back to page 1 whenever the items per page change. So not only if the items per page increase, but instead on any change

Copy link
Member

Choose a reason for hiding this comment

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

I thought we already had this kind of reset in the past. Don't know if it got lost when we moved the pagination params from the store into the composable+query. 🤔 do you know or did you find any orphaned code fragments regarding the page reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO we should go back to page 1 whenever the items per page change. So not only if the items per page increase, but instead on any change

Sounds good as well 👍

do you know or did you find any orphaned code fragments regarding the page reset?

I didn't see anything regarding this. Where could such code be located? AFAIK ViewOptions is the only component where we set items-per-page.

Copy link
Member

Choose a reason for hiding this comment

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

can't find anything either

return router.replace({
query: {
...unref(currentRoute).query,
'items-per-page': itemsPerPage,
...(resetPagination && { page: '1' })
}
})
}

const setViewMode = (mode: ViewMode) => {
viewModeQuery.value = mode.name
}

watch(
[itemsPerPageQuery, viewModeQuery, viewSizeQuery],
(params) => {
Expand All @@ -154,19 +187,15 @@ export default defineComponent({
viewSizeCurrent: viewSizeQuery,
itemsPerPageCurrent: itemsPerPageQuery,
queryParamsLoading,
setTilesViewSize
setTilesViewSize,
setItemsPerPage,
setViewMode,
viewOptionsButtonLabel: $gettext('Display customization options of the files list')
}
},
computed: {
...mapState('Files', ['areHiddenFilesShown', 'areFileExtensionsShown']),

viewOptionsButtonLabel() {
return this.$gettext('Display customization options of the files list')
},
resizeTilesLabel() {
return this.$gettext('Tile size')
},

hiddenFilesShownModel: {
get() {
return this.areHiddenFilesShown
Expand All @@ -189,12 +218,7 @@ export default defineComponent({
methods: {
queryItemAsString,
...mapMutations('Files', ['SET_HIDDEN_FILES_VISIBILITY', 'SET_FILE_EXTENSIONS_VISIBILITY']),
setViewMode(mode) {
this.viewModeCurrent = mode.name
},
setItemsPerPage(itemsPerPage) {
this.itemsPerPageCurrent = itemsPerPage
},

updateHiddenFilesShownModel(event) {
this.hiddenFilesShownModel = event
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
import ViewOptions from 'web-app-files/src/components/AppBar/ViewOptions.vue'
import { useRouteQueryPersisted } from 'web-pkg/src/composables/router'
import { ref, nextTick } from 'vue'
import { useRouteQueryPersisted, useRouteQuery } from 'web-pkg/src/composables/router'
import { ref } from 'vue'
import {
createStore,
defaultPlugins,
defaultStoreMockOptions,
defaultComponentMocks,
mount
mount,
RouteLocation
} from 'web-test-helpers'
import { ViewModeConstants } from 'web-app-files/src/composables'
import { mock } from 'jest-mock-extended'

jest.mock('web-pkg/src/composables/router', () => ({
...jest.requireActual('web-pkg/src/composables/router'),
useRouteQueryPersisted: jest.fn(),
useRouteQuery: jest.fn()
}))

jest.mock('web-pkg/src/composables/router')
const selectors = {
pageSizeSelect: '.oc-page-size',
hiddenFilesSwitch: '[data-testid="files-switch-hidden-files"]',
Expand All @@ -30,14 +37,26 @@ describe('ViewOptions component', () => {
const { wrapper } = getWrapper({ perPage })
expect(wrapper.findComponent<any>(selectors.pageSizeSelect).props().selected).toBe(perPage)
})
it('sets the correct files page limit', async () => {
it('sets the correct files page limit', () => {
const perPage = '100'
const newItemsPerPage = '500'
const { wrapper } = getWrapper({ perPage })
const { wrapper, mocks } = getWrapper({ perPage })
wrapper.vm.setItemsPerPage(newItemsPerPage)
expect(mocks.$router.replace).toHaveBeenCalledWith(
expect.objectContaining({
query: expect.objectContaining({ 'items-per-page': newItemsPerPage })
})
)
})
it('resets the page to 1 if current page is > 1 and amount of items per page is being increased', () => {
const perPage = '100'
const newItemsPerPage = '500'
const { wrapper, mocks } = getWrapper({ perPage, currentPage: '2' })
wrapper.vm.setItemsPerPage(newItemsPerPage)
await nextTick()
expect(wrapper.findComponent<any>(selectors.pageSizeSelect).props().selected).toBe(
newItemsPerPage
expect(mocks.$router.replace).toHaveBeenCalledWith(
expect.objectContaining({
query: expect.objectContaining({ 'items-per-page': newItemsPerPage, page: '1' })
})
)
})
})
Expand Down Expand Up @@ -109,15 +128,19 @@ function getWrapper({
perPage = '100',
viewMode = ViewModeConstants.default.name,
tileSize = '1',
props = {}
props = {},
currentPage = '1'
} = {}) {
jest.mocked(useRouteQueryPersisted).mockImplementationOnce(() => ref(perPage))
jest.mocked(useRouteQueryPersisted).mockImplementationOnce(() => ref(viewMode))
jest.mocked(useRouteQueryPersisted).mockImplementationOnce(() => ref(tileSize))
jest.mocked(useRouteQuery).mockImplementationOnce(() => ref(currentPage))

const storeOptions = { ...defaultStoreMockOptions }
const store = createStore(storeOptions)
const mocks = { ...defaultComponentMocks() }
const mocks = {
...defaultComponentMocks({ currentRoute: mock<RouteLocation>({ path: '/files' }) })
}
return {
storeOptions,
mocks,
Expand Down