Bug 25086 - OPAC Self Registration - Field 'changed_fields' doesn't have a default value
Summary: OPAC Self Registration - Field 'changed_fields' doesn't have a default value
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Agustín Moyano
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 23579
Blocks: 23816
  Show dependency treegraph
 
Reported: 2020-04-08 19:52 UTC by Agustín Moyano
Modified: 2020-05-05 23:08 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 25086: set changed_fields column if not defined (1.08 KB, patch)
2020-04-08 20:01 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25086: set changed_fields column if not defined (1.13 KB, patch)
2020-04-14 07:16 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25086: set changed_fields column if not defined (2.31 KB, patch)
2020-04-15 16:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25086: (follow-up) Set changed_fields column as nullable (1.65 KB, patch)
2020-04-15 16:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25086: (follow-up) Set changed_fields column as nullable (1.70 KB, patch)
2020-04-15 21:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25086: set changed_fields column if not defined (2.30 KB, patch)
2020-04-16 16:12 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25086: (follow-up) Set changed_fields column as nullable (1.70 KB, patch)
2020-04-16 16:12 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25086: set changed_fields column if not defined (1.13 KB, patch)
2020-04-16 18:25 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25086: (follow-up) Set changed_fields column as nullable (2.75 KB, patch)
2020-04-16 18:25 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25086: set changed_fields column if not defined (1.13 KB, patch)
2020-04-16 20:03 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25086: (follow-up) Set changed_fields column as nullable (2.80 KB, patch)
2020-04-16 20:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25086: Set changed_fields column as nullable (2.38 KB, patch)
2020-04-17 05:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2020-04-08 19:52:14 UTC
In OPAC's self registration, if PatronSelfRegistrationVerifyByEmail is set to Require, OPAC dies with the following error

DBD::mysql::st execute failed: Field 'changed_fields' doesn't have a default value [for Statement "INSERT INTO `borrower_modifications` ( `branchcode`, `categorycode`, `date_renewed`, `dateenrolled`, `dateexpiry`, `dateofbirth`, `debarred`, `email`, `firstname`, `gdpr_proc_consent`, `password`, `surname`, `timestamp`, `verification_token`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, current_timestamp, ? )" with ParamValues: 0="CPL", 1="S", 2=undef, 3=undef, 4=undef, 5=undef, 6=undef, 7="prueba2@algo.com", 8="algo", 9=undef, 10='E<rcyc6h<p', 11="prueba2", 12='aa165f462c22f414a2cd1081bace0921'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.

Seems it's because changed_fields doesn't have a default value, and cannot be null.
Comment 1 Agustín Moyano 2020-04-08 20:01:31 UTC
Created attachment 102600 [details] [review]
Bug 25086: set changed_fields column if not defined

This patch sets changed_fields column to '' if not defined

To test:
1. Do not apply this patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and restart_all
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Comment 2 Jonathan Druart 2020-04-09 09:27:24 UTC
Agustin, at first glance I'd say that the correct fix is to make the column nullable at DB level.
Comment 3 Victor Grousset/tuxayo 2020-04-14 07:16:44 UTC
Created attachment 102893 [details] [review]
Bug 25086: set changed_fields column if not defined

This patch sets changed_fields column to '' if not defined

To test:
1. Do not apply this patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and restart_all
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 4 Victor Grousset/tuxayo 2020-04-14 07:18:25 UTC
Patch signed off.
Comment 5 Victor Grousset/tuxayo 2020-04-14 07:19:39 UTC
However, the message of Joubu seems to mean that it doesn't pass QA.
Comment 6 Agustín Moyano 2020-04-15 16:58:18 UTC
Created attachment 103024 [details] [review]
Bug 25086: set changed_fields column if not defined

This patch sets changed_fields column to '' if not defined

To test:
1. Do not apply this patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and restart_all
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Agustín Moyano 2020-04-15 16:58:22 UTC
Created attachment 103025 [details] [review]
Bug 25086: (follow-up) Set changed_fields column as nullable

This patch reverts previous patch (left there because it was signed off), and sets changed_fields column as nullable.
Attend only this test plan and not previous patch.

To test:
1. Do not apply any patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and updatedatabase
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Comment 8 Victor Grousset/tuxayo 2020-04-15 21:59:27 UTC
Created attachment 103038 [details] [review]
Bug 25086: (follow-up) Set changed_fields column as nullable

This patch reverts previous patch (left there because it was signed off), and sets changed_fields column as nullable.
Attend only this test plan and not previous patch.

To test:
1. Do not apply any patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and updatedatabase
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2020-04-15 22:04:47 UTC
It works! 👌
Comment 10 Jonathan Druart 2020-04-16 09:13:00 UTC
Something is wrong here

error: sha1 information is lacking or useless (installer/data/mysql/atomicupdate/bug_25086.perl).
error: could not build fake ancestor
Patch failed at 0001 Bug 25086: (follow-up) Set changed_fields column as nullable
Comment 11 Victor Grousset/tuxayo 2020-04-16 16:12:00 UTC
Created attachment 103093 [details] [review]
Bug 25086: set changed_fields column if not defined

This patch sets changed_fields column to '' if not defined

To test:
1. Do not apply this patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and restart_all
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 12 Victor Grousset/tuxayo 2020-04-16 16:12:03 UTC
Created attachment 103094 [details] [review]
Bug 25086: (follow-up) Set changed_fields column as nullable

This patch reverts previous patch (left there because it was signed off), and sets changed_fields column as nullable.
Attend only this test plan and not previous patch.

To test:
1. Do not apply any patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and updatedatabase
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 13 Victor Grousset/tuxayo 2020-04-16 16:18:11 UTC
/me forgot to attach the second patch.

Fixed, sorry for hassle.

SelfRegistration retested again just in case. Nothing suspicious found.
Comment 14 Agustín Moyano 2020-04-16 18:25:50 UTC
Created attachment 103095 [details] [review]
Bug 25086: set changed_fields column if not defined

This patch sets changed_fields column to '' if not defined

To test:
1. Do not apply this patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and restart_all
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Agustín Moyano 2020-04-16 18:25:54 UTC
Created attachment 103096 [details] [review]
Bug 25086: (follow-up) Set changed_fields column as nullable

This patch reverts previous patch (left there because it was signed off), and sets changed_fields column as nullable.
Attend only this test plan and not previous patch.

To test:
1. Do not apply any patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and updatedatabase
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Comment 16 Agustín Moyano 2020-04-16 18:27:38 UTC
(In reply to Jonathan Druart from comment #10)
> Something is wrong here
> 
> error: sha1 information is lacking or useless
> (installer/data/mysql/atomicupdate/bug_25086.perl).
> error: could not build fake ancestor
> Patch failed at 0001 Bug 25086: (follow-up) Set changed_fields column as
> nullable

You were right.. somehow I messed up my rebase and replaced original signed off commit with another.

I just put things in order.

Please try again.

Thanks
Comment 17 Victor Grousset/tuxayo 2020-04-16 19:52:53 UTC
Wait I'm sure I was the one who messed up.

Because my command history shows that I only attached the last patch when we must reattach all patches each time. (IIUC)

And because to do the signoff I was able to apply the 2 patches without issue. It was only after my signoff that Joubu wasn't able to apply the 2 patches.

I'm rechecking what you just submitted Agustín.
Comment 18 Victor Grousset/tuxayo 2020-04-16 20:03:57 UTC
Created attachment 103097 [details] [review]
Bug 25086: set changed_fields column if not defined

This patch sets changed_fields column to '' if not defined

To test:
1. Do not apply this patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and restart_all
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Victor Grousset/tuxayo 2020-04-16 20:04:00 UTC
Created attachment 103098 [details] [review]
Bug 25086: (follow-up) Set changed_fields column as nullable

This patch reverts previous patch (left there because it was signed off), and sets changed_fields column as nullable.
Attend only this test plan and not previous patch.

To test:
1. Do not apply any patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and updatedatabase
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 20 Victor Grousset/tuxayo 2020-04-16 20:11:18 UTC
Working as expected.

Agustín: I don't think that two patches are needed. From what I understand, if you change your strategy, you don't need a commit revert the V1. Nothing is released. So just discard or amend your commit and reattach it to the ticket.

In the current state, I think you can just squash the two commits. And keep the commit message of the 2nd commit, without the part saying that it reverts the 1st one.

That's my current understanding.
Comment 21 Marcel de Rooy 2020-04-17 05:49:58 UTC
Created attachment 103115 [details] [review]
Bug 25086: Set changed_fields column as nullable

This patch changed_fields column as nullable.

To test:
1. Do not apply any patch
2. Set PatronSelfRegistrationVerifyByEmail preference to require
3. Fill self registration form in opac and submit
CHECK => OPAC dies with a nasty message, that says Field 'changed_fields' doesn't have a default value
4. Apply this patch and updatedatabase
5. Repeat step 3
SUCCESS => Registry process continues as expected
6. Sign off

Sponsored-by: Orex Digital
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2020-04-17 05:50:34 UTC
(In reply to Victor Grousset/tuxayo from comment #20)
> Working as expected.
> 
> Agustín: I don't think that two patches are needed. From what I understand,
> if you change your strategy, you don't need a commit revert the V1. Nothing
> is released. So just discard or amend your commit and reattach it to the
> ticket.
> 
> In the current state, I think you can just squash the two commits. And keep
> the commit message of the 2nd commit, without the part saying that it
> reverts the 1st one.
> 
> That's my current understanding.

Agreed
Comment 23 Martin Renvoize 2020-04-17 08:20:39 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 24 Joy Nelson 2020-05-05 23:08:18 UTC
backported to 19.11.x for 19.11.06