|
Description
paxed
2014-04-29 10:33:56 UTC
Should be removed and functionality added to Koha::Patron Created attachment 72902 [details] [review] Bug 12159: Move borrower_add_additional_fields to Koha::Patron The same function was effectively coded twice. It was removed from members/pay.pl and members/paycollect.pl and placed in Koha/Patron.pm instead. Haven't written the tests yet, so feel free to take it, write them, and ask for sign off. :) Created attachment 73018 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Created attachment 73019 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t The problem is a bit complicated, this sub was wrong at all (sorry for mystification you Mark). I added new method Koha::Patron->attributes and use it instead I couldn't apply the patch on master branch (17.12.00.037) : Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 12159: Fix getting extended patron attributes for circ-menu Using index info to reconstruct a base tree... M Koha/Patron.pm M circ/circulation.pl M koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc M koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt M koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt M members/boraccount.pl M members/member-flags.pl M members/memberentry.pl M members/moremember.pl M members/paycollect.pl Falling back to patching base and 3-way merge... Auto-merging members/paycollect.pl Auto-merging members/moremember.pl Auto-merging members/memberentry.pl Auto-merging members/member-flags.pl Auto-merging members/boraccount.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc Auto-merging circ/circulation.pl Auto-merging Koha/Patron.pm CONFLICT (content): Merge conflict in Koha/Patron.pm Failed to merge in the changes. Patch failed at 0001 Bug 12159: Fix getting extended patron attributes for circ-menu The copy of the patch that failed is found in: /var/repositories/koha/.git/rebase-apply/patch Created attachment 74781 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Created attachment 74782 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t Created attachment 74791 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 74792 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Attributes checked "Display in check-out" are now displayed on side panel of all tabs in patron account (that's useful). Maybe the description of this option on the "Attribute menu" should be modifiy now. "Display in check-out: Check to show this attribute in patron check-out." --> "Display in patron's brief information" ? (In reply to Séverine Queune from comment #12) > Attributes checked "Display in check-out" are now displayed on side panel of > all tabs in patron account (that's useful). > > Maybe the description of this option on the "Attribute menu" should be > modifiy now. > "Display in check-out: Check to show this attribute in patron check-out." > --> "Display in patron's brief information" ? Definitely makes sense to me, would you mind to fill a bug for this? (In reply to Josef Moravec from comment #13) > (In reply to Séverine Queune from comment #12) > > Attributes checked "Display in check-out" are now displayed on side panel of > > all tabs in patron account (that's useful). > > > > Maybe the description of this option on the "Attribute menu" should be > > modifiy now. > > "Display in check-out: Check to show this attribute in patron check-out." > > --> "Display in patron's brief information" ? > > Definitely makes sense to me, would you mind to fill a bug for this? No problem ! Done with Bug 20648. Created attachment 75811 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 75812 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 75822 [details] [review] Bug 12159: (QA follow-up) Silencing POD warn from qa tools *** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm FAIL Koha/Patron/Attribute.pm forbidden pattern: Warning: The 'sub type' may be wrong is declared in a Koha::* package (see bug 15446) (line 127) Josef: This is a more nasty one. Could you consider renaming this method? The Koha objects also use _type and it might be confusing. What do you think? (In reply to Marcel de Rooy from comment #18) > FAIL Koha/Patron/Attribute.pm > forbidden pattern: Warning: The 'sub type' may be wrong is declared in a > Koha::* package (see bug 15446) (line 127) > > Josef: > This is a more nasty one. > Could you consider renaming this method? > The Koha objects also use _type and it might be confusing. > What do you think? It already exists. IMO we should push this one along with bug 20443 (which does not have patches yet). Otherwise we are going to have 2 different ways to fetch patron's attributes. Before we only had GetBorrowerAttributes. I agree with Jonathan here (In reply to Marcel de Rooy from comment #18) > FAIL Koha/Patron/Attribute.pm > forbidden pattern: Warning: The 'sub type' may be wrong is declared in a > Koha::* package (see bug 15446) (line 127) > > Josef: > This is a more nasty one. > Could you consider renaming this method? > The Koha objects also use _type and it might be confusing. > What do you think? We could rename it of course, but isn't that the reason why the type in Koha::Object was renamed to _type? To avoid conflicts with type accessor if needed? (In reply to Jonathan Druart from comment #20) > IMO we should push this one along with bug 20443 (which does not have > patches yet). Otherwise we are going to have 2 different ways to fetch > patron's attributes. > Before we only had GetBorrowerAttributes. Does make sense. I'll try to look at it. How can we make progress here? Waiting for patches for bug 20443? Created attachment 76945 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 76946 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 76947 [details] [review] Bug 12159: (QA follow-up) Silencing POD warn from qa tools *** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm Patches rebased. Why BLOCKED? I believe Tomas is working on bug 20443.. which I think is the blocker here. (In reply to Katrin Fischer from comment #29) > Why BLOCKED? See comment 20. (In reply to Martin Renvoize from comment #30) > I believe Tomas is working on bug 20443.. which I think is the blocker here. See assignee field, I am working on it. Please do not change anything here. Created attachment 77050 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 77051 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 77052 [details] [review] Bug 12159: (QA follow-up) Silencing POD warn from qa tools *** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm Conflicts with bug 20805 fixed. Created attachment 86166 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 86167 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 86168 [details] [review] Bug 12159: (QA follow-up) Silencing POD warn from qa tools *** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm Rebased Comment on attachment 86168 [details] [review] Bug 12159: (QA follow-up) Silencing POD warn from qa tools No longer needed QA Comment: Saw this warning once: Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc line 84. False warning from qa tools: Koha/Patron/Attribute.pm critic/forbidden patterns: forbidden pattern: Warning: The 'sub type' may be wrong is declared in a Koha::* package (see bug 15446) (line 127) A dumb question: + return Koha::Patron::Attributes->search({ + borrowernumber => $self->borrowernumber, + branchcode => $self->branchcode, + }); => Is the branchcode needed? Koha/Patron/Attribute.pm L114 return $av->next->lib if $av->count Amended: adding a semicolon for future ease. Koha/Schema/Result/BorrowerAttribute.pm => Please do not add DBIx schema files in a regular patch. I removed it now. Btw it seems no longer necessary if we rename the relation in the Koha object. See follow-up. Created attachment 86882 [details] [review] Bug 12159: Fix getting extended patron attributes for circ-menu Changes: - Replace getting preference ExtendedPatronAttributes by Koha.Preference in templates - Add Koha::Patron->attributes for getting patrons extended attributes - Use this method in circ-menu.inc - Remove getting attributes from members perl scripts Test plan: 0) Apply the patch 1) Add some patron attributes type - with free text, authorised value, limited by libraries... 2) Add some values to this attributes for some patrons 3) Go through as many patron pages as you can and confirm that attributes are shown at side panel when they shoul and are not shown when they should not be shown Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Removed Koha/Schema/Result/BorrowerAttribute.pm [EDIT] Added missing semicolon on L114 in Koha/Patron/Attribute.pm Created attachment 86883 [details] [review] Bug 12159: Add tests Test plan: prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 86884 [details] [review] Bug 12159: (QA follow-up) Rename relation in Patron::Attributes It seems that we do not need the extra belongs_to in the schema result files if we rename the relation to code. Test plan: Run t/db_dependent/Koha/Patrons.t Run t/db_dependent/Koha/Patron/Attributes.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 86885 [details] [review] Bug 12159: (QA follow-up) Remove double use module Speaks for itself. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Discussion is unclear - this now blocks 20443 - do we want these pushed together, or can this one go now and then be followed by 20443 when it is ready? Awesome work all! Pushed to master for 19.05 Architectural enhancement will not be backported to 18.11.x series. |