Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions)
Summary: Move auto member cardnumber generation to occur when record is "Saved" (avoid...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Meenakshi
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 7068
  Show dependency treegraph
 
Reported: 2011-08-25 04:46 UTC by Savitra Sirohi
Modified: 2017-06-14 22:18 UTC (History)
9 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:


Attachments
Attaching Patch. (3.59 KB, patch)
2012-06-06 11:44 UTC, Meenakshi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions). (3.67 KB, patch)
2012-07-03 12:45 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions). (5.99 KB, patch)
2016-09-30 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions). (6.34 KB, patch)
2016-09-30 20:32 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions). (6.37 KB, patch)
2016-12-14 12:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions). (6.42 KB, patch)
2016-12-30 18:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6782 [QA Followup] - Remove unused param and limit calls to Koha.Preference (4.44 KB, patch)
2016-12-30 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6782: Fix fixup_cardnumber call (1.15 KB, patch)
2017-01-16 09:09 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Savitra Sirohi 2011-08-25 04:46:22 UTC
Currently the card number is generated when the user enters the patron creation form. This creates a problem of concurrency - when two or more simulataneous users are registering members, the error "card no. in use" can occur.

This change moves the card number generation to occur after the "Save" button is pressed.
Comment 1 Gaetan Boisson 2012-05-22 10:44:29 UTC
Confirmed.

The method described by Savitra is the way it works when generating barcodes for items and it should indeed be the one used to generate card numbers as well.
Comment 2 Meenakshi 2012-06-06 11:44:43 UTC Comment hidden (obsolete)
Comment 3 Michael Davis 2012-06-10 14:37:21 UTC
The field is still marked as required, so fails the javascript validation on form submit. The comment under the field would be better if it followed the style under the other form elements - <div class="hint">
Comment 4 Gaetan Boisson 2012-07-03 12:43:02 UTC
Michael, you can choose which fields are mandatory in the BorrowerMandatoryField syspref, cardnumber is mandatory by default.

So with this patch if you activate autoMemberNum then you need to remove cardnumber form BorrowerMandatoryField. I would say either we just add it in the text that goes with the syspref :
"Do / Don't default the card number field on the patron addition screen to the next available card number (for example, if the largest currently used card number is 26345000012941, then this field will default to 26345000012942). ++Remember to remove cardnumber from the BorrowerMandatoryField syspref if you switch this on++",
 or we remove it from BorrowerMandatoryField's default value (i think this would be a bad idea), or we find a way to bypass the mandatory behaviour on this field when autoMemberNum is on (This sounds like a complicated idea).

I'll sign this off for now, and leave it to the QA team to decide wether we need to add something to make things clearer.
Comment 5 Gaetan Boisson 2012-07-03 12:45:21 UTC
Created attachment 10625 [details] [review]
[SIGNED-OFF] Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions).

Currently the card number is generated when the user enters the patron creation form. This creates a problem of concurrency - when two or more simulataneous users are registering members, the error "card no. in use" can occur.

This change moves the card number generation to occur after the "Save" button is pressed.

Changes:
-C4/Members.pm:
Added code to fixup_cardnumber,If the cardnumber is blank and "autoMemberNum" ON.
-koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:
Added code to display "leave blank for auto calc during registration" in cardnumber label in patron registration form only if "autoMemberNum" ON.
-members/memberentry.pl:
Added code to get weather or not "autoMemberNum" is on or off and removed fixup_cardnumber generation.

Test cases:
-If "autoMemberNum" ON:
->In blank case, must generate auto card number in simulataneous users.
->If user entered, check for unique card number.

-If "autoMemberNum" OFF:
Must work normal.

Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>
Comment 6 Paul Poulain 2012-07-10 13:50:54 UTC
Comment on attachment 9945 [details] [review]
Attaching Patch.

This patch obsoleted by the signed-off one
Comment 7 Paul Poulain 2012-07-10 13:53:42 UTC
QA comment: some Perl comments:
+    if(C4::Context->preference("autoMemberNum")) {
+        if ($data{'cardnumber'} eq '') {
+               $data{'cardnumber'}= fixup_cardnumber($data{'cardnumber'},C4::Context->userenv->{'branch'}, $data{'categorycode'});
+        }
+    }
=> you should write
+    if (C4::Context->preference("autoMemberNum") && $data{'cardnumber'} eq '') {
+               $data{'cardnumber'}= fixup_cardnumber($data{'cardnumber'},C4::Context->userenv->{'branch'}, $data{'categorycode'});
+    }

and
+if (C4::Context->preference("autoMemberNum")) {
+    $template->param( autoMemberNum => 1);
+} else {
+    $template->param( autoMemberNum => 0);
+}
could be set 
+ $template->param( autoMemberNum => C4::Context->preference("autoMemberNum"));

Marking failed QA for that reason, please fix & resubmit, i'll push quickly
Comment 8 Marc Véron 2016-05-23 14:55:09 UTC
Seems still to be an issue.

To reproduce:
- Set syspref autoMemberNum to 'on'
- Open two browser windows/tabs and go in both to 
  Home > Patrons > Add patron
- Verify that in both entry forms the field 'Card number' is 
  populated with the same value
- Save form in browser window 1
=>OK
- Save form in browser window 2
=> Result in window 2: Message "Cardnumber already in use.",
   and the field 'Card number' is pupulated with the next higher number.
   Saving again is then OK
Comment 9 Marc Véron 2016-08-23 07:57:28 UTC
- Still an issue on current master, see comment #8
- Patch does not apply
Comment 10 Jonathan Druart 2016-09-30 09:39:46 UTC
Created attachment 55952 [details] [review]
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions).

Currently the card number is generated when the user enters the patron creation form. This creates a problem of concurrency - when two or more simulataneous users are registering members, the error "card no. in use" can occur.

This change moves the card number generation to occur after the "Save" button is pressed.

Changes:
-C4/Members.pm:
Added code to fixup_cardnumber,If the cardnumber is blank and "autoMemberNum" ON.
-koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:
Added code to display "leave blank for auto calc during registration" in cardnumber label in patron registration form only if "autoMemberNum" ON.
-members/memberentry.pl:
Added code to get weather or not "autoMemberNum" is on or off and removed fixup_cardnumber generation.

Test cases:
-If "autoMemberNum" ON:
->In blank case, must generate auto card number in simulataneous users.
->If user entered, check for unique card number.

-If "autoMemberNum" OFF:
Must work normal.
Comment 11 Jonathan Druart 2016-09-30 09:40:58 UTC
Revived patch, tests will need to be provided for changes to C4::Members::AddMember, but everything is testable.
Comment 12 Marc Véron 2016-09-30 20:32:24 UTC
Created attachment 55964 [details] [review]
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions).

Currently the card number is generated when the user enters the patron creation form. This creates a problem of concurrency - when two or more simulataneous users are registering members, the error "card no. in use" can occur.

This change moves the card number generation to occur after the "Save" button is pressed.

Changes:
-C4/Members.pm:
Added code to fixup_cardnumber,If the cardnumber is blank and "autoMemberNum" ON.
-koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:
Added code to display "leave blank for auto calc during registration" in cardnumber label in patron registration form only if "autoMemberNum" ON.
-members/memberentry.pl:
Added code to get weather or not "autoMemberNum" is on or off and removed fixup_cardnumber generation.

Test cases:
-If "autoMemberNum" ON:
->In blank case, must generate auto card number in simulataneous users.
->If user entered, check for unique card number.

-If "autoMemberNum" OFF:
Must work normal.

Followed test plan, works as expected.
Note: Syspref PorrowerMandatoryField must not include cardnumber, otherwise
      you can not save. Maybe that should be mentioned in the comment for
      syspref autoMemberNum.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 13 Jonathan Druart 2016-10-03 11:57:30 UTC
QAers: See (potential) answers of bug 17215 comment 11.
Comment 14 Jonathan Druart 2016-12-14 12:43:36 UTC
Created attachment 58173 [details] [review]
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions).

Currently the card number is generated when the user enters the patron creation form. This creates a problem of concurrency - when two or more simulataneous users are registering members, the error "card no. in use" can occur.

This change moves the card number generation to occur after the "Save" button is pressed.

Changes:
-C4/Members.pm:
Added code to fixup_cardnumber,If the cardnumber is blank and "autoMemberNum" ON.
-koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:
Added code to display "leave blank for auto calc during registration" in cardnumber label in patron registration form only if "autoMemberNum" ON.
-members/memberentry.pl:
Added code to get weather or not "autoMemberNum" is on or off and removed fixup_cardnumber generation.

Test cases:
-If "autoMemberNum" ON:
->In blank case, must generate auto card number in simulataneous users.
->If user entered, check for unique card number.

-If "autoMemberNum" OFF:
Must work normal.

Followed test plan, works as expected.
Note: Syspref PorrowerMandatoryField must not include cardnumber, otherwise
      you can not save. Maybe that should be mentioned in the comment for
      syspref autoMemberNum.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 15 Kyle M Hall 2016-12-30 18:42:56 UTC
Created attachment 58544 [details] [review]
Bug 6782 - Move auto member cardnumber generation to occur when record is "Saved" (avoid collisions).

Currently the card number is generated when the user enters the patron creation form. This creates a problem of concurrency - when two or more simulataneous users are registering members, the error "card no. in use" can occur.

This change moves the card number generation to occur after the "Save" button is pressed.

Changes:
-C4/Members.pm:
Added code to fixup_cardnumber,If the cardnumber is blank and "autoMemberNum" ON.
-koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:
Added code to display "leave blank for auto calc during registration" in cardnumber label in patron registration form only if "autoMemberNum" ON.
-members/memberentry.pl:
Added code to get weather or not "autoMemberNum" is on or off and removed fixup_cardnumber generation.

Test cases:
-If "autoMemberNum" ON:
->In blank case, must generate auto card number in simulataneous users.
->If user entered, check for unique card number.

-If "autoMemberNum" OFF:
Must work normal.

Followed test plan, works as expected.
Note: Syspref PorrowerMandatoryField must not include cardnumber, otherwise
      you can not save. Maybe that should be mentioned in the comment for
      syspref autoMemberNum.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2016-12-30 18:43:07 UTC
Created attachment 58545 [details] [review]
Bug 6782 [QA Followup] - Remove unused param and limit calls to Koha.Preference

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2017-01-13 11:46:55 UTC
Pushed to master for 17.05, thanks Meenakshi!
Comment 18 Jonathan Druart 2017-01-13 16:29:35 UTC
This broke t/db_dependent/Reserves.t

t/db_dependent/Reserves.t .. 1/72 Can't use an undefined value as a HASH reference at /home/vagrant/kohaclone/C4/Members.pm line 502.

Please fix ASAP
Comment 19 Jonathan Druart 2017-01-13 16:31:07 UTC
(In reply to Jonathan Druart from comment #18)
> This broke t/db_dependent/Reserves.t
> 
> t/db_dependent/Reserves.t .. 1/72 Can't use an undefined value as a HASH
> reference at /home/vagrant/kohaclone/C4/Members.pm line 502.
> 
> Please fix ASAP

And 17 other files.
Comment 20 Katrin Fischer 2017-01-15 18:10:35 UTC
Has broken tests and is an enhancement - not going to be in 16.11.02.
Comment 21 Jonathan Druart 2017-01-16 09:09:50 UTC
Created attachment 59002 [details] [review]
Bug 6782: Fix fixup_cardnumber call

The fixup_cardnumber subroutine takes only 1 parameter, the cardnumber.
This call is wrong and morevover makes a lot of tests fail:

t/db_dependent/Letters.t .. 1/79 Can't use an undefined value as a HASH
reference at /home/vagrant/kohaclone/C4/Members.pm line 502.

This happens because the userenv is not mocked in a lot of test files.
Comment 22 Kyle M Hall 2017-01-18 11:47:34 UTC
Followup pushed to master!