Merge branch...

Merge branch '57991-frontend-pagination-needs-to-handle-cases-where-the-x-total-pages-header-isn-t-present' into 'master'

Improve the JS pagination to handle the case when the `X-Total` and `X-Total-Pages` headers aren't present

Closes #57991

See merge request gitlab-org/gitlab-ce!25601

app/assets/javascripts/pipelines/mixins/pipelines.js

@@ -27,11 +27,7 @@ export default {
   },
   computed: {
     shouldRenderPagination() {
-      return (
-        !this.isLoading &&
-        this.state.pipelines.length &&
-        this.state.pageInfo.total > this.state.pageInfo.perPage
-      );
+      return !this.isLoading;
     },
   },
   beforeMount() {

app/assets/javascripts/vue_shared/components/table_pagination.vue

@@ -54,15 +54,14 @@ export default {
       return this.pageInfo.nextPage;
     },
     getItems() {
-      const total = this.pageInfo.totalPages;
-      const { page } = this.pageInfo;
+      const { totalPages, nextPage, previousPage, page } = this.pageInfo;
       const items = [];
 
       if (page > 1) {
         items.push({ title: FIRST, first: true });
       }
 
-      if (page > 1) {
+      if (previousPage) {
         items.push({ title: PREV, prev: true });
       } else {
         items.push({ title: PREV, disabled: true, prev: true });
@@ -70,32 +69,34 @@ export default {
 
       if (page > UI_LIMIT) items.push({ title: SPREAD, separator: true });
 
+      if (totalPages) {
         const start = Math.max(page - PAGINATION_UI_BUTTON_LIMIT, 1);
-      const end = Math.min(page + PAGINATION_UI_BUTTON_LIMIT, total);
+        const end = Math.min(page + PAGINATION_UI_BUTTON_LIMIT, totalPages);
 
         for (let i = start; i <= end; i += 1) {
           const isActive = i === page;
           items.push({ title: i, active: isActive, page: true });
         }
 
-      if (total - page > PAGINATION_UI_BUTTON_LIMIT) {
+        if (totalPages - page > PAGINATION_UI_BUTTON_LIMIT) {
           items.push({ title: SPREAD, separator: true, page: true });
         }
+      }
 
-      if (page === total) {
-        items.push({ title: NEXT, disabled: true, next: true });
-      } else if (total - page >= 1) {
+      if (nextPage) {
         items.push({ title: NEXT, next: true });
+      } else {
+        items.push({ title: NEXT, disabled: true, next: true });
       }
 
-      if (total - page >= 1) {
+      if (totalPages && totalPages - page >= 1) {
         items.push({ title: LAST, last: true });
       }
 
       return items;
     },
     showPagination() {
-      return this.pageInfo.totalPages > 1;
+      return this.pageInfo.nextPage || this.pageInfo.previousPage;
     },
   },
   methods: {

changelogs/unreleased/57991-frontend-pagination-needs-to-handle-cases-where-the-x-total-pages-header-isn-t-present.yml

+---
+title: "Improve the JS pagination to handle the case when the `X-Total` and `X-Total-Pages` headers aren't present"
+merge_request: 25601
+author:
+type: fixed

spec/javascripts/vue_shared/components/table_pagination_spec.js

@@ -22,10 +22,10 @@ describe('Pagination component', () => {
     it('should not render anything', () => {
       component = mountComponent({
         pageInfo: {
-          nextPage: 1,
+          nextPage: NaN,
           page: 1,
           perPage: 20,
-          previousPage: null,
+          previousPage: NaN,
           total: 15,
           totalPages: 1,
         },
@@ -58,6 +58,28 @@ describe('Pagination component', () => {
         expect(spy).not.toHaveBeenCalled();
       });
 
+      it('should be disabled and non clickable when total and totalPages are NaN', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: 2,
+            page: 1,
+            perPage: 20,
+            previousPage: NaN,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        expect(
+          component.$el.querySelector('.js-previous-button').classList.contains('disabled'),
+        ).toEqual(true);
+
+        component.$el.querySelector('.js-previous-button .page-link').click();
+
+        expect(spy).not.toHaveBeenCalled();
+      });
+
       it('should be enabled and clickable', () => {
         component = mountComponent({
           pageInfo: {
@@ -75,6 +97,24 @@ describe('Pagination component', () => {
 
         expect(spy).toHaveBeenCalledWith(1);
       });
+
+      it('should be enabled and clickable when total and totalPages are NaN', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: 3,
+            page: 2,
+            perPage: 20,
+            previousPage: 1,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        component.$el.querySelector('.js-previous-button .page-link').click();
+
+        expect(spy).toHaveBeenCalledWith(1);
+      });
     });
 
     describe('first button', () => {
@@ -99,6 +139,28 @@ describe('Pagination component', () => {
 
         expect(spy).toHaveBeenCalledWith(1);
       });
+
+      it('should call the change callback with the first page when total and totalPages are NaN', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: 3,
+            page: 2,
+            perPage: 20,
+            previousPage: 1,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        const button = component.$el.querySelector('.js-first-button .page-link');
+
+        expect(button.textContent.trim()).toEqual('« First');
+
+        button.click();
+
+        expect(spy).toHaveBeenCalledWith(1);
+      });
     });
 
     describe('last button', () => {
@@ -123,16 +185,32 @@ describe('Pagination component', () => {
 
         expect(spy).toHaveBeenCalledWith(5);
       });
+
+      it('should not render', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: 3,
+            page: 2,
+            perPage: 20,
+            previousPage: 1,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        expect(component.$el.querySelector('.js-last-button .page-link')).toBeNull();
+      });
     });
 
     describe('next button', () => {
       it('should be disabled and non clickable', () => {
         component = mountComponent({
           pageInfo: {
-            nextPage: 5,
+            nextPage: NaN,
             page: 5,
             perPage: 20,
-            previousPage: 1,
+            previousPage: 4,
             total: 84,
             totalPages: 5,
           },
@@ -146,6 +224,26 @@ describe('Pagination component', () => {
         expect(spy).not.toHaveBeenCalled();
       });
 
+      it('should be disabled and non clickable when total and totalPages are NaN', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: NaN,
+            page: 5,
+            perPage: 20,
+            previousPage: 4,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        expect(component.$el.querySelector('.js-next-button').textContent.trim()).toEqual('Next');
+
+        component.$el.querySelector('.js-next-button .page-link').click();
+
+        expect(spy).not.toHaveBeenCalled();
+      });
+
       it('should be enabled and clickable', () => {
         component = mountComponent({
           pageInfo: {
@@ -163,6 +261,24 @@ describe('Pagination component', () => {
 
         expect(spy).toHaveBeenCalledWith(4);
       });
+
+      it('should be enabled and clickable when total and totalPages are NaN', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: 4,
+            page: 3,
+            perPage: 20,
+            previousPage: 2,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        component.$el.querySelector('.js-next-button .page-link').click();
+
+        expect(spy).toHaveBeenCalledWith(4);
+      });
     });
 
     describe('numbered buttons', () => {
@@ -181,9 +297,26 @@ describe('Pagination component', () => {
 
         expect(component.$el.querySelectorAll('.page').length).toEqual(5);
       });
+
+      it('should not render any page', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: 4,
+            page: 3,
+            perPage: 20,
+            previousPage: 2,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        expect(component.$el.querySelectorAll('.page').length).toEqual(0);
+      });
     });
 
-    it('should render the spread operator', () => {
+    describe('spread operator', () => {
+      it('should render', () => {
         component = mountComponent({
           pageInfo: {
             nextPage: 4,
@@ -198,5 +331,22 @@ describe('Pagination component', () => {
 
         expect(component.$el.querySelector('.separator').textContent.trim()).toEqual('...');
       });
+
+      it('should not render', () => {
+        component = mountComponent({
+          pageInfo: {
+            nextPage: 4,
+            page: 3,
+            perPage: 20,
+            previousPage: 2,
+            total: NaN,
+            totalPages: NaN,
+          },
+          change: spy,
+        });
+
+        expect(component.$el.querySelector('.separator')).toBeNull();
+      });
+    });
   });
 });