Bug 35942

Summary: OPAC user can enroll several times to the same club
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: OPACAssignee: Kyle M Hall <kyle>
Status: Pushed to oldoldoldstable --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: dcook, didier.gautheron, f.demians, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, laurent.ducos, lucas, m.de.rooy, martin.renvoize, nick, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03,23.05.09,22.11.15,22.05.19
Bug Depends on: 35941, 12461    
Bug Blocks:    
Attachments: Bug 35942: OPAC user can enroll several times to the same club
Bug 35942: OPAC user can enroll several times to the same club
Bug 35942: OPAC user can enroll several times to the same club
Bug 35942: OPAC user can enroll several times to the same club
Bug 35942: OPAC user can enroll several times to the same club [23.05.x]

Description Jonathan Druart 2024-01-30 14:00:38 UTC
There is no restriction (certainly a unique key missing).

To recreate:
Create 3 clubs, 1 limited to library A, 1 limited to library B and one not limited

Use a patron with home library A.

Go to the opac-user page, "Clubs" tab show 0/2 (the one from library B is not listed)

Hit
http://localhost:8080/cgi-bin/koha/svc/club/enroll?id=1
http://localhost:8080/cgi-bin/koha/svc/club/enroll?id=2
http://localhost:8080/cgi-bin/koha/svc/club/enroll?id=3

Oops, you see 3/0

Hit again the same link

Oops, you see 6/0
Comment 1 Jonathan Druart 2024-01-30 14:01:43 UTC
There are 2 things here:
1. No unicity check on (patron, club, date_cancelled)
2. No check on the library limitation
Comment 2 Kyle M Hall 2024-01-30 16:03:18 UTC
Created attachment 161648 [details] [review]
Bug 35942: OPAC user can enroll several times to the same club

Test Plan:

1) Create 3 clubs, 1 limited to library A, 1 limited to library B and one not limited
2) Use a patron with home library A.
3) Go to the opac-user page, "Clubs" tab show 0/2 (the one from library B is not listed)
4) Browse to /cgi-bin/koha/svc/club/enroll?id=1
5) Reload that page a couple times
6) Note the patron is now enrolled in the same club multiple times
7) Delete those enrollments
8) Apply this patch
9) Restart all the things!
10) Repeat steps 2-7, note the lack of duplicate enrollments!
11) Repeat steps 2-10 for the staff interface
Comment 3 Kyle M Hall 2024-01-30 16:05:16 UTC
Created attachment 161649 [details] [review]
Bug 35942: OPAC user can enroll several times to the same club

Test Plan:

1) Create 3 clubs, 1 limited to library A, 1 limited to library B and one not limited
2) Use a patron with home library A.
3) Go to the opac-user page, "Clubs" tab show 0/2 (the one from library B is not listed)
4) Browse to /cgi-bin/koha/svc/club/enroll?id=1
5) Reload that page a couple times
6) Note the patron is now enrolled in the same club multiple times
7) Delete those enrollments
8) Apply this patch
9) Restart all the things!
10) Repeat steps 2-7, note the lack of duplicate enrollments!
11) Repeat steps 2-10 for the staff interface
Comment 4 Jonathan Druart 2024-01-31 09:02:06 UTC
Created attachment 161665 [details] [review]
Bug 35942: OPAC user can enroll several times to the same club

Test Plan:

1) Create 3 clubs, 1 limited to library A, 1 limited to library B and one not limited
2) Use a patron with home library A.
3) Go to the opac-user page, "Clubs" tab show 0/2 (the one from library B is not listed)
4) Browse to /cgi-bin/koha/svc/club/enroll?id=1
5) Reload that page a couple times
6) Note the patron is now enrolled in the same club multiple times
7) Delete those enrollments
8) Apply this patch
9) Restart all the things!
10) Repeat steps 2-7, note the lack of duplicate enrollments!
11) Repeat steps 2-10 for the staff interface

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Jonathan Druart 2024-01-31 09:02:32 UTC
Not perfect but good enough for now. We should have the constraint in the DB.

Also I am wondering if the start/end dates should not be checked as well. That's for another day...

Thanks for the quick fix!
Comment 6 Martin Renvoize 2024-01-31 13:51:44 UTC
Created attachment 161689 [details] [review]
Bug 35942: OPAC user can enroll several times to the same club

Test Plan:

1) Create 3 clubs, 1 limited to library A, 1 limited to library B and one not limited
2) Use a patron with home library A.
3) Go to the opac-user page, "Clubs" tab show 0/2 (the one from library B is not listed)
4) Browse to /cgi-bin/koha/svc/club/enroll?id=1
5) Reload that page a couple times
6) Note the patron is now enrolled in the same club multiple times
7) Delete those enrollments
8) Apply this patch
9) Restart all the things!
10) Repeat steps 2-7, note the lack of duplicate enrollments!
11) Repeat steps 2-10 for the staff interface

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2024-01-31 13:52:03 UTC
All working, QA script happy, Passing QA
Comment 8 Kyle M Hall 2024-01-31 14:07:53 UTC
(In reply to Jonathan Druart from comment #5)
> Not perfect but good enough for now. We should have the constraint in the DB.
> 
> Also I am wondering if the start/end dates should not be checked as well.
> That's for another day...
> 
> Thanks for the quick fix!

I wasn't sure which columns should be part of a UNIQUE constraint for this table. It definitely needs club_id, borrowernumber, and date_cancelled. I'm not sure if date_cancelled being nullable causes any issues. If you think those are a good key set I'll add a followup to enforce those.
Comment 9 Jonathan Druart 2024-02-07 08:32:19 UTC
(In reply to Kyle M Hall from comment #8)
> (In reply to Jonathan Druart from comment #5)
> > Not perfect but good enough for now. We should have the constraint in the DB.
> > 
> > Also I am wondering if the start/end dates should not be checked as well.
> > That's for another day...
> > 
> > Thanks for the quick fix!
> 
> I wasn't sure which columns should be part of a UNIQUE constraint for this
> table. It definitely needs club_id, borrowernumber, and date_cancelled. I'm
> not sure if date_cancelled being nullable causes any issues. If you think
> those are a good key set I'll add a followup to enforce those.

Hum indeed, we certainly need a separate column (boolean) to tell that the patron is currently enrolled or not.
Comment 10 Martin Renvoize 2024-02-15 09:47:26 UTC
Pushed to master for 24.05.00
Comment 11 Lucas Gass 2024-02-21 21:52:34 UTC
This doesn't apply cleanly to 23.05.x. Kyle can we get a rebase?
Comment 12 Kyle M Hall 2024-02-22 14:17:19 UTC
Created attachment 162332 [details] [review]
Bug 35942: OPAC user can enroll several times to the same club [23.05.x]

Test Plan:

1) Create 3 clubs, 1 limited to library A, 1 limited to library B and one not limited
2) Use a patron with home library A.
3) Go to the opac-user page, "Clubs" tab show 0/2 (the one from library B is not listed)
4) Browse to /cgi-bin/koha/svc/club/enroll?id=1
5) Reload that page a couple times
6) Note the patron is now enrolled in the same club multiple times
7) Delete those enrollments
8) Apply this patch
9) Restart all the things!
10) Repeat steps 2-7, note the lack of duplicate enrollments!
11) Repeat steps 2-10 for the staff interface

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 wainuiwitikapark 2024-02-24 01:28:14 UTC
Hi there, I can't seem to reproduce the error on 22.05.x
Comment 14 Jonathan Druart 2024-02-26 10:55:21 UTC
(In reply to wainuiwitikapark from comment #13)
> Hi there, I can't seem to reproduce the error on 22.05.x

I do reproduce on 22.05.x
Comment 15 Fridolin Somers 2024-03-04 10:46:08 UTC
I see it is in 22.05.19

@Frederic

But I don't see it in 22.11.15
Comment 16 Frédéric Demians 2024-03-04 11:09:43 UTC
(In reply to Fridolin Somers from comment #15)

> But I don't see it in 22.11.15

Noooo! I don't know what happened. I can see myself testing this patch.
Comment 17 Fridolin Somers 2024-03-06 15:12:36 UTC
Patch pushed in v22.11.15-1