When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. AFTER THIS PATCH: Check_Userid() is not called when modifying or saving partial borrower properties. Userid is changed using a separate module.
Created attachment 35404 [details] [review] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. AFTER THIS PATCH: Check_Userid() is not called when modifying or saving partial borrower properties.
Can you please describe how to reproduce this bug?
I don't have time to test this thoroughly, but I can confirm the problem: If you use the small 'edit' links for a section of the patron record instead of the 'edit' link in the toolbar, you can't save changes - as you get an error message: The following fields are wrong. Please fix them. Username/password already exists.
Created attachment 35435 [details] [review] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. AFTER THIS PATCH: Check_Userid() get the $userid from the new parameters, or if not present, from the koha.borrowers-table.
Created attachment 35436 [details] [review] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. REPLICATE THE ISSUE: 1. Find a Borrower. 2. Go to the details-tab. 3. Under "Patron messaging preferences" click edit to edit just the messaging preferences. 4. Just save, no need to make any modifications. 5. Observe the error "Username/password already exists" This happens to all/most of the sub-forms in the Borrower's Details-view. AFTER THIS PATCH: Check_Userid() get the $userid from the new parameters, or if not present, from the koha.borrowers-table. TEST FOR REGRESSION: 1. Edit a borrower normally. Notice no errors. 2. Edit a sub-form (Alternate address/Patron messaging preferences). Notice no errors.
Created attachment 35437 [details] [review] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. REPLICATE THE ISSUE: 1. Find a Borrower. 2. Go to the details-tab. 3. Under "Patron messaging preferences" click edit to edit just the messaging preferences. 4. Just save, no need to make any modifications. 5. Observe the error "Username/password already exists" This happens to all/most of the sub-forms in the Borrower's Details-view. AFTER THIS PATCH: Check_Userid() get the $userid from the new parameters, or if not present, from the koha.borrowers-table. TEST FOR REGRESSION: 1. Edit a borrower normally. Notice no errors. 2. Edit a sub-form (Alternate address/Patron messaging preferences). Notice no errors. DISCLAIMER: You are free to steal/rip/modify/hack this patch to your soul's extent without my permission. As long as it makes to Koha I am happy.
*** Bug 13649 has been marked as a duplicate of this bug. ***
Created attachment 35648 [details] [review] [Signed-off] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. REPLICATE THE ISSUE: 1. Find a Borrower. 2. Go to the details-tab. 3. Under "Patron messaging preferences" click edit to edit just the messaging preferences. 4. Just save, no need to make any modifications. 5. Observe the error "Username/password already exists" This happens to all/most of the sub-forms in the Borrower's Details-view. AFTER THIS PATCH: Check_Userid() get the $userid from the new parameters, or if not present, from the koha.borrowers-table. TEST FOR REGRESSION: 1. Edit a borrower normally. Notice no errors. 2. Edit a sub-form (Alternate address/Patron messaging preferences). Notice no errors. DISCLAIMER: You are free to steal/rip/modify/hack this patch to your soul's extent without my permission. As long as it makes to Koha I am happy. Works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 35650 [details] [review] [PASSED QA] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. REPLICATE THE ISSUE: 1. Find a Borrower. 2. Go to the details-tab. 3. Under "Patron messaging preferences" click edit to edit just the messaging preferences. 4. Just save, no need to make any modifications. 5. Observe the error "Username/password already exists" This happens to all/most of the sub-forms in the Borrower's Details-view. AFTER THIS PATCH: Check_Userid() get the $userid from the new parameters, or if not present, from the koha.borrowers-table. TEST FOR REGRESSION: 1. Edit a borrower normally. Notice no errors. 2. Edit a sub-form (Alternate address/Patron messaging preferences). Notice no errors. DISCLAIMER: You are free to steal/rip/modify/hack this patch to your soul's extent without my permission. As long as it makes to Koha I am happy. Works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 35651 [details] [review] [PASSED QA] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences When editing a Patron's messaging preferences or borrower attributes, one cannot save changes because 'userid' is not given as a parameter. It shouldn't, but the Check_Userid($newdata{'userid'},$borrowernumber) -check must have it. REPLICATE THE ISSUE: 1. Find a Borrower. 2. Go to the details-tab. 3. Under "Patron messaging preferences" click edit to edit just the messaging preferences. 4. Just save, no need to make any modifications. 5. Observe the error "Username/password already exists" This happens to all/most of the sub-forms in the Borrower's Details-view. AFTER THIS PATCH: Check_Userid() get the $userid from the new parameters, or if not present, from the koha.borrowers-table. TEST FOR REGRESSION: 1. Edit a borrower normally. Notice no errors. 2. Edit a sub-form (Alternate address/Patron messaging preferences). Notice no errors. DISCLAIMER: You are free to steal/rip/modify/hack this patch to your soul's extent without my permission. As long as it makes to Koha I am happy. Works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment on attachment 35651 [details] [review] [PASSED QA] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences Review of attachment 35651 [details] [review]: ----------------------------------------------------------------- ::: members/memberentry.pl @@ +329,5 @@ > } > } > + # Check if the userid is unique. Userid might not always be present in the edited values list when editing certain sub-forms, so we need to dig for it a bit. > + my $userid = $newdata{'userid'}; > + $userid = $borrower_data->{userid} unless $userid; #Get the userid straight from the DB In my opinion, my $userid = $newdata{userid} ? $newdata{userid} : $borrower_data->{userid}; or my $userid = $newdata{userid}; $userid ||= $borrower_data->{userid}; Would have been more readable.
Ran into another problem here - if you leave the password/userid empty on creating a new organisation type patron, you get an error message. It should totally be possible to create users with no login or let Koha create the userid automatically. (Filed a new bug 13655)
(In reply to Jonathan Druart from comment #11) > Comment on attachment 35651 [details] [review] [review] > [PASSED QA] Bug 13602 - Username/password already exists when editing > borrower attributes and messaging preferences > > Review of attachment 35651 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: members/memberentry.pl > @@ +329,5 @@ > > } > > } > > + # Check if the userid is unique. Userid might not always be present in the edited values list when editing certain sub-forms, so we need to dig for it a bit. > > + my $userid = $newdata{'userid'}; > > + $userid = $borrower_data->{userid} unless $userid; #Get the userid straight from the DB > > In my opinion, > > my $userid = $newdata{userid} ? $newdata{userid} : > $borrower_data->{userid}; > > or > > my $userid = $newdata{userid}; > $userid ||= $borrower_data->{userid}; > > Would have been more readable. We could even use the defined-or operator: my $userid = $newdata->{ userid } // $borrower_data->{ userid };
Patch pushed to master. Thanks Olli!
Pushed to 3.18.x will be in 3.18.4
(In reply to Chris Cormack from comment #15) > Pushed to 3.18.x will be in 3.18.4 Pushed to 3.16.x, will be in 3.16.8
Depends on Bug 13502 which changed the behavior of Check_Userid with undefined $uid. Not needed for 3.14.x. By the way, why check userid when this data is unchanged (not in $newdata because not edited) ? Also, the flag $NoUpdateLogin manages whether or not the userid can be edited.