Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences
Summary: Username/password already exists when editing borrower attributes and messagi...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Whiteboard:
Keywords: rel_3_16_candidate
: 13649 (view as bug list)
Depends on: 13502
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-20 14:41 UTC by Olli-Antti Kivilahti
Modified: 2015-12-03 22:00 UTC (History)
9 users (show)

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


Attachments
Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences (1.30 KB, patch)
2015-01-20 14:49 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences (1.43 KB, patch)
2015-01-21 12:36 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences (1.92 KB, patch)
2015-01-21 12:43 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences (2.07 KB, patch)
2015-01-21 12:45 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[Signed-off] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences (2.23 KB, patch)
2015-02-01 10:54 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences (2.28 KB, patch)
2015-02-01 21:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13602 - Username/password already exists when editing borrower attributes and messaging preferences (2.28 KB, patch)
2015-02-01 21:48 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2015-01-20 14:41:54 UTC
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.
Comment 1 Olli-Antti Kivilahti 2015-01-20 14:49:39 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2015-01-20 19:23:24 UTC
Can you please describe how to reproduce this bug?
Comment 3 Katrin Fischer 2015-01-20 22:41:30 UTC
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.
Comment 4 Olli-Antti Kivilahti 2015-01-21 12:36:05 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2015-01-21 12:43:42 UTC Comment hidden (obsolete)
Comment 6 Olli-Antti Kivilahti 2015-01-21 12:45:36 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2015-01-30 18:56:16 UTC
*** Bug 13649 has been marked as a duplicate of this bug. ***
Comment 8 Marc Véron 2015-02-01 10:54:57 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2015-02-01 21:45:07 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2015-02-01 21:48:21 UTC
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 11 Jonathan Druart 2015-02-02 16:37:43 UTC
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.
Comment 12 Katrin Fischer 2015-02-02 16:45:21 UTC
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)
Comment 13 Tomás Cohen Arazi 2015-02-09 13:51:11 UTC
(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 };
Comment 14 Tomás Cohen Arazi 2015-02-09 16:48:00 UTC
Patch pushed to master.

Thanks Olli!
Comment 15 Chris Cormack 2015-02-12 21:40:25 UTC
Pushed to 3.18.x will be in 3.18.4
Comment 16 Mason James 2015-02-26 03:43:19 UTC
(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
Comment 17 Fridolin SOMERS 2015-04-02 09:33:27 UTC
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.