Bug 18008 - Use templates for generating pagination in intranet
Summary: Use templates for generating pagination in intranet
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Josef Moravec
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18005
Blocks: 18030
  Show dependency treegraph
 
Reported: 2017-01-28 08:11 UTC by Josef Moravec
Modified: 2018-09-26 18:26 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18008: Make pagination generated in templates (24.35 KB, patch)
2017-02-02 08:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18008: Make pagination generated in templates (22.60 KB, patch)
2017-05-03 15:15 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18008: Make pagination generated in templates (22.62 KB, patch)
2017-10-04 05:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18008: Make pagination generated in templates (22.63 KB, patch)
2017-10-06 07:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18008: Make pagination generated in templates (21.77 KB, patch)
2017-10-06 14:16 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18008: Make pagination generated in templates (22.02 KB, patch)
2018-03-28 15:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18008: Make pagination generated in templates (21.79 KB, patch)
2018-09-25 11:25 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2017-01-28 08:11:14 UTC
Except catalogue search results the pagination is generated by C4::Output::pagination_bar, id don't think it's the right way.
Comment 1 David Cook 2017-02-02 01:16:40 UTC
I'm not sure I understand this bug, but I'm very much in favour of creating more standardized ways of paginating in Koha, especially in the staff client...
Comment 2 Josef Moravec 2017-02-02 08:47:07 UTC Comment hidden (obsolete)
Comment 3 Josef Moravec 2017-02-02 08:52:17 UTC
(In reply to David Cook from comment #1)
> I'm not sure I understand this bug, but I'm very much in favour of creating
> more standardized ways of paginating in Koha, especially in the staff
> client...

Thank for your interest, any feedback is appreciated, of course ;)

Here is my first attempt to make pagination a bit more usable.
Comment 4 Josef Moravec 2017-05-03 15:15:29 UTC Comment hidden (obsolete)
Comment 5 Alex Buckley 2017-10-04 02:24:09 UTC
Hi Josef

When I apply this patch on an up to date clean branch I get the following error:

Applying: Bug 18008: Make pagination generated in templates
Using index info to reconstruct a base tree...
M	acqui/neworderbiblio.pl
M	cataloguing/value_builder/marc21_linking_section.pl
M	cataloguing/value_builder/unimarc_field_4XX.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/matching-rules.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
M	reports/guided_reports.pl
M	reviews/reviewswaiting.pl
M	tags/review.pl
M	virtualshelves/shelves.pl
Falling back to patching base and 3-way merge...
Auto-merging virtualshelves/shelves.pl
Auto-merging tags/review.pl
Auto-merging reviews/reviewswaiting.pl
CONFLICT (content): Merge conflict in reviews/reviewswaiting.pl
Auto-merging reports/guided_reports.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/matching-rules.tt
Auto-merging cataloguing/value_builder/unimarc_field_4XX.pl
Auto-merging cataloguing/value_builder/marc21_linking_section.pl
Auto-merging acqui/neworderbiblio.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 18008: Make pagination generated in templates
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-18008-Make-pagination-generated-in-templates-1W76fJ.patch
Comment 6 Josef Moravec 2017-10-04 05:18:49 UTC Comment hidden (obsolete)
Comment 7 Josef Moravec 2017-10-04 05:19:35 UTC
(In reply to Alex Buckley from comment #5)
> Hi Josef
> 
> When I apply this patch on an up to date clean branch I get the following
> error:
> 

Thanks for testing, I've just rebased on master, should be OK now...
Comment 8 Alex Buckley 2017-10-05 20:44:08 UTC
Hi Josef

Thanks for rebasing, I have tested the patch and functionally everything works well, however running QA test tools it fails for pagination.inc (see the below test output). Can you please fix this up and then I'll be happy to sign off on this patch.


 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/pagination.inc
   FAIL	  forbidden patterns
		forbidden pattern: Do not use line breaks inside template tags (bug 18675) (line 1)
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template
Comment 9 Josef Moravec 2017-10-06 07:18:16 UTC
Created attachment 67672 [details] [review]
Bug 18008: Make pagination generated in templates

It generates pagination in bootstrap style (as bug 18005 also do)

Modified pages:
acqui/neworderbiblio.pl
authorities/authorities-home.pl
cataloguing/addbooks.pl
cataloguing/value_builder/marc21_linking_section.pl
cataloguing/value_builder/unimarc_field_210c.pl
cataloguing/value_builder/unimarc_field_4XX.pl
reports/guided_reports.pl
reviews/reviewswaiting.pl
tags/review.pl
virtualshelves/shelves.pl

Test plan:
0) Apply the patch
1) go through pages from the list above/use cataloguing plugins, and
make the data to be big enough for pagination
2) confirm that pagination looks nice and behave correctly
3) git grep pagination_bar
  should return just C4/Output.pm and opac templates (I'll deal with it
          on another bugs)
Comment 10 Josef Moravec 2017-10-06 07:18:47 UTC
(In reply to Alex Buckley from comment #8)
> Hi Josef
> 
> Thanks for rebasing, I have tested the patch and functionally everything
> works well, however running QA test tools it fails for pagination.inc (see
> the below test output). Can you please fix this up and then I'll be happy to
> sign off on this patch.
> 
> 
>  FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/pagination.inc
>    FAIL	  forbidden patterns
> 		forbidden pattern: Do not use line breaks inside template tags (bug 18675)
> (line 1)
>    OK	  git manipulation
>    OK	  spelling
>    OK	  tt_valid
>    OK	  valid_template

It was because of comments in pagination.inc, now fixed, thanks
Comment 11 Owen Leonard 2017-10-06 13:19:37 UTC
I didn't get through the whole test plan, but I ran into a few things:

In the template for reviews/reviewswaiting.pl the pagination bar is picking up some style conflicts from the container it's in. I suggest moving it outside div.statictabs.

I don't see that pagination is used by tags/review.pl or reports/guided_reports.pl because they're in DataTables now. Can the code be removed?
Comment 12 Josef Moravec 2017-10-06 14:16:29 UTC
Created attachment 67717 [details] [review]
Bug 18008: Make pagination generated in templates

It generates pagination in bootstrap style (as bug 18005 also do)

Modified pages:
acqui/neworderbiblio.pl
authorities/authorities-home.pl
cataloguing/addbooks.pl
cataloguing/value_builder/marc21_linking_section.pl
cataloguing/value_builder/unimarc_field_210c.pl
cataloguing/value_builder/unimarc_field_4XX.pl
reports/guided_reports.pl
reviews/reviewswaiting.pl
tags/review.pl
virtualshelves/shelves.pl

Test plan:
0) Apply the patch
1) go through pages from the list above/use cataloguing plugins, and
make the data to be big enough for pagination
2) confirm that pagination looks nice and behave correctly
3) git grep pagination_bar
  should return just C4/Output.pm and opac templates (I'll deal with it
          on another bugs)
Comment 13 Josef Moravec 2017-10-06 14:19:59 UTC
(In reply to Owen Leonard from comment #11)
> I didn't get through the whole test plan, but I ran into a few things:
> 
> In the template for reviews/reviewswaiting.pl the pagination bar is picking
> up some style conflicts from the container it's in. I suggest moving it
> outside div.statictabs.
> 
> I don't see that pagination is used by tags/review.pl or
> reports/guided_reports.pl because they're in DataTables now. Can the code be
> removed?

Thanks for testing, I tried to address all this issues in new patch
Comment 14 Josef Moravec 2018-03-28 15:47:36 UTC
Created attachment 73398 [details] [review]
Bug 18008: Make pagination generated in templates

It generates pagination in bootstrap style (as bug 18005 also do)

Modified pages:
acqui/neworderbiblio.pl
authorities/authorities-home.pl
cataloguing/addbooks.pl
cataloguing/value_builder/marc21_linking_section.pl
cataloguing/value_builder/unimarc_field_210c.pl
cataloguing/value_builder/unimarc_field_4XX.pl
reports/guided_reports.pl
reviews/reviewswaiting.pl
tags/review.pl
virtualshelves/shelves.pl

Test plan:
0) Apply the patch
1) go through pages from the list above/use cataloguing plugins, and
make the data to be big enough for pagination
2) confirm that pagination looks nice and behave correctly
3) git grep pagination_bar
  should return just C4/Output.pm and opac templates (I'll deal with it
          on another bugs)
Comment 15 Josef Moravec 2018-03-28 15:47:57 UTC
Just rebased on master
Comment 16 Michal Denar 2018-09-25 11:05:35 UTC
It call for rebase :-)
Comment 17 Josef Moravec 2018-09-25 11:25:32 UTC
Created attachment 79366 [details] [review]
Bug 18008: Make pagination generated in templates

It generates pagination in bootstrap style (as bug 18005 also do)

Modified pages:
acqui/neworderbiblio.pl
authorities/authorities-home.pl
cataloguing/addbooks.pl
cataloguing/value_builder/marc21_linking_section.pl
cataloguing/value_builder/unimarc_field_210c.pl
cataloguing/value_builder/unimarc_field_4XX.pl
reports/guided_reports.pl
reviews/reviewswaiting.pl
tags/review.pl
virtualshelves/shelves.pl

Test plan:
0) Apply the patch
1) go through pages from the list above/use cataloguing plugins, and
make the data to be big enough for pagination
2) confirm that pagination looks nice and behave correctly
3) git grep pagination_bar
  should return just C4/Output.pm and opac templates (I'll deal with it
          on another bugs)
Comment 18 Josef Moravec 2018-09-25 11:26:04 UTC
Rebased on and adapted to master ;)
Comment 19 Owen Leonard 2018-09-26 18:26:34 UTC
> cataloguing/value_builder/unimarc_field_210c.pl

I don't see any pagination on this page.

> cataloguing/value_builder/unimarc_field_4XX.pl
> cataloguing/value_builder/marc21_linking_section.pl

Pagination appears to have been removed altogether from these pages, but I don't think that's correct.