|
Description
Marcel de Rooy
2018-07-12 13:40:18 UTC
Created attachment 77037 [details] [review] Bug 21066: Database revision *Convert* timestamp to publisheddate, add updated_on as 'real' timestamp. Created attachment 77038 [details] [review] Bug 21066: Code changes needed for replacing opac_news.timestamp Note: This is not finished yet. TODO installer/data/mysql/en/mandatory/sample_notices.sql:<p class="newsfooter" style="font-size: 8pt; font-style:italic; margin-bottom: 1px; margin-top: 1px">Posted on <<opac_news.timestamp>></p> TODO installer/data/mysql/en/optional/sample_news.sql:INSERT INTO `opac_news` (`title`, `new`, `lang`, `timestamp`, `expirationdate`, `number`) ETC TODO t/db_dependent/NewsChannels.t Created attachment 77039 [details] [review] Bug 21066: DBIx changes for opac_news Well, this is just a preview of my proposal. Replace timestamp by publisheddate (a real date) and add updated_on (a real timestamp). These are currently 16.11 patches. I hope to work later on moving them upwards, depending on community interest and increased momentum on other proposed developments.. Hi Marcel, I think maybe have: updated_on created_on And use the publication date from the entry in the notices? With the TT syntax being more widely available the formatting should not be as problematic as it used to be. I am not so happy about publisheddate because the date the entry is published (visible) might be quite different from the date the entry was created. Hope I understood correctly! Argh... should have looked more closely at the table. I completely understand your problem now. I think making the timestamp a publishingdate (better fit with expirationdate and might be in the future) and having another real timestamp field makes totally sense. Created attachment 107769 [details] [review] Bug 21066: Database revision Change timestamp to publicationdate, add updated_on as 'real' timestamp. Test plan: Run install or upgrade. Check table opac_news. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 107770 [details] [review] DO_NOT_PUSH: DBIx schema changes Created attachment 107771 [details] [review] Bug 21066: Code changes needed for replacing opac_news.timestamp Test plan: Perform some CRUD operations on the news page in staff. Verify that news on opac and staff respect publicationdate. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 107772 [details] [review] Bug 21066: Resolve SQL errors in strict mode DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_master`.`opac_news`.`number` at row 1 [for Statement "INSERT INTO opac_news ( content,number,lang,title,branchcode,borrowernumber,expirationdate,publicationdate ) VALUES ( ?,?,?,?,?,?,?,? )" with ParamValues: 0="test", 1="", 2="", 3="test", 4=undef, 5="51", 6='2020-08-11', 7='2020-08-04'] at /usr/share/koha/C4/NewsChannels.pm line 68. DBD::mysql::st execute failed: Incorrect integer value: 'a' for column `koha_master`.`opac_news`.`number` at row 1 [for Statement "UPDATE opac_news SET lang = ?,publicationdate = ?,expirationdate = ?,number = ?,content = ?,branchcode = ?,title = ? WHERE idnew = ?;" with ParamValues: 0="", 1='2020-08-03', 2=undef, 3="a", 4="test", 5=undef, 6="test", 7="9"] at /usr/share/koha/C4/NewsChannels.pm line 107. Test plan: Do not apply this patch. Enable strict mode in koha-conf. Add new record in tools/news leaving 'Appear in position' blank. The record is not added; you have a warn in your plack-error log. Apply patch and try again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 107773 [details] [review] Bug 21066: Update NewsChannels.t Removing the weird '' => 1 from the hash too. NOTE: The following line in the module seems to be useless: $data->{$data->{'lang'}} = 1 if defined $data->{lang}; Test plan: Run t/db_dependent/NewsChannels.t Run t/db_dependent/Koha/News.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 107774 [details] [review] Bug 21066: Update English sample notices Applies to sample notices and sample news. Replace timestamp by publicationdate. Also adjust notice in TemplateToolkit.t. Test plan: Run dbrev and verify if ISSUESLIP refers to opac_news.publicationdate. Run t/db_dependent/Letters/TemplateToolkit.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 107775 [details] [review] Bug 21066: Update translated news and notices Replace timestamp by publicationdate in translations. Applies to sample notices and sample news. Test plan: Run a new install or pick a sql file and run it on cmdline. (In reply to Katrin Fischer from comment #6) > Argh... should have looked more closely at the table. I completely > understand your problem now. > > I think making the timestamp a publishingdate (better fit with > expirationdate and might be in the future) and having another real timestamp > field makes totally sense. Okay, second try. With publicationdate. Perhaps even a bit better than publishingdate ;) Created attachment 107776 [details] [review] Bug 21066: Update translated news and notices Replace timestamp by publicationdate in translations. Applies to sample notices and sample news. Test plan: Run a new install or pick a sql file and run it on cmdline. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 109148 [details] [review] Bug 21066: Database revision Change timestamp to publicationdate, add updated_on as 'real' timestamp. Test plan: Run install or upgrade. Check table opac_news. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 109149 [details] [review] DO_NOT_PUSH: DBIx schema changes https://bugs.koha-community.org/show_bug.cgi?id=21066 Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 109150 [details] [review] Bug 21066: Code changes needed for replacing opac_news.timestamp Test plan: Perform some CRUD operations on the news page in staff. Verify that news on opac and staff respect publicationdate. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 109151 [details] [review] Bug 21066: Resolve SQL errors in strict mode DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_master`.`opac_news`.`number` at row 1 [for Statement "INSERT INTO opac_news ( content,number,lang,title,branchcode,borrowernumber,expirationdate,publicationdate ) VALUES ( ?,?,?,?,?,?,?,? )" with ParamValues: 0="test", 1="", 2="", 3="test", 4=undef, 5="51", 6='2020-08-11', 7='2020-08-04'] at /usr/share/koha/C4/NewsChannels.pm line 68. DBD::mysql::st execute failed: Incorrect integer value: 'a' for column `koha_master`.`opac_news`.`number` at row 1 [for Statement "UPDATE opac_news SET lang = ?,publicationdate = ?,expirationdate = ?,number = ?,content = ?,branchcode = ?,title = ? WHERE idnew = ?;" with ParamValues: 0="", 1='2020-08-03', 2=undef, 3="a", 4="test", 5=undef, 6="test", 7="9"] at /usr/share/koha/C4/NewsChannels.pm line 107. Test plan: Do not apply this patch. Enable strict mode in koha-conf. Add new record in tools/news leaving 'Appear in position' blank. The record is not added; you have a warn in your plack-error log. Apply patch and try again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 109152 [details] [review] Bug 21066: Update NewsChannels.t Removing the weird '' => 1 from the hash too. NOTE: The following line in the module seems to be useless: $data->{$data->{'lang'}} = 1 if defined $data->{lang}; Test plan: Run t/db_dependent/NewsChannels.t Run t/db_dependent/Koha/News.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 109153 [details] [review] Bug 21066: Update English sample notices Applies to sample notices and sample news. Replace timestamp by publicationdate. Also adjust notice in TemplateToolkit.t. Test plan: Run dbrev and verify if ISSUESLIP refers to opac_news.publicationdate. Run t/db_dependent/Letters/TemplateToolkit.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 109154 [details] [review] Bug 21066: Update translated news and notices Replace timestamp by publicationdate in translations. Applies to sample notices and sample news. Test plan: Run a new install or pick a sql file and run it on cmdline. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 109192 [details] [review] Bug 21066: Database revision Change timestamp to publicationdate, add updated_on as 'real' timestamp. Test plan: Run install or upgrade. Check table opac_news. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109193 [details] [review] DO_NOT_PUSH: DBIx schema changes https://bugs.koha-community.org/show_bug.cgi?id=21066 Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109194 [details] [review] Bug 21066: Code changes needed for replacing opac_news.timestamp Test plan: Perform some CRUD operations on the news page in staff. Verify that news on opac and staff respect publicationdate. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109195 [details] [review] Bug 21066: Resolve SQL errors in strict mode DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_master`.`opac_news`.`number` at row 1 [for Statement "INSERT INTO opac_news ( content,number,lang,title,branchcode,borrowernumber,expirationdate,publicationdate ) VALUES ( ?,?,?,?,?,?,?,? )" with ParamValues: 0="test", 1="", 2="", 3="test", 4=undef, 5="51", 6='2020-08-11', 7='2020-08-04'] at /usr/share/koha/C4/NewsChannels.pm line 68. DBD::mysql::st execute failed: Incorrect integer value: 'a' for column `koha_master`.`opac_news`.`number` at row 1 [for Statement "UPDATE opac_news SET lang = ?,publicationdate = ?,expirationdate = ?,number = ?,content = ?,branchcode = ?,title = ? WHERE idnew = ?;" with ParamValues: 0="", 1='2020-08-03', 2=undef, 3="a", 4="test", 5=undef, 6="test", 7="9"] at /usr/share/koha/C4/NewsChannels.pm line 107. Test plan: Do not apply this patch. Enable strict mode in koha-conf. Add new record in tools/news leaving 'Appear in position' blank. The record is not added; you have a warn in your plack-error log. Apply patch and try again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109196 [details] [review] Bug 21066: Update NewsChannels.t Removing the weird '' => 1 from the hash too. NOTE: The following line in the module seems to be useless: $data->{$data->{'lang'}} = 1 if defined $data->{lang}; Test plan: Run t/db_dependent/NewsChannels.t Run t/db_dependent/Koha/News.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109197 [details] [review] Bug 21066: Update English sample notices Applies to sample notices and sample news. Replace timestamp by publicationdate. Also adjust notice in TemplateToolkit.t. Test plan: Run dbrev and verify if ISSUESLIP refers to opac_news.publicationdate. Run t/db_dependent/Letters/TemplateToolkit.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109198 [details] [review] Bug 21066: Update translated news and notices Replace timestamp by publicationdate in translations. Applies to sample notices and sample news. Test plan: Run a new install or pick a sql file and run it on cmdline. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109199 [details] [review] Bug 21066: (QA follow-up) Fix publication date display in OPAC The date would no longer show, changed the variable name and removed the hours, as the field in the database is a date and not a datetime. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Katrin Fischer from comment #30) > Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Thanks Hm, was checking this for a similar bug I am working on and there are 2 issues with the first database update: 1) It should check for the new column existing (idempotent) 2) It should specify where the new column should be added with AFTER Created attachment 109354 [details] [review] Bug 21066: (QA follow-up) Check for field existence in dbrev Test plan: Run the upgrade twice. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Katrin Fischer from comment #32) > Hm, was checking this for a similar bug I am working on and there are 2 > issues with the first database update: > > 1) It should check for the new column existing (idempotent) > 2) It should specify where the new column should be added with AFTER Thx for noting. Fixed Comment on attachment 109192 [details] [review] Bug 21066: Database revision Review of attachment 109192 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/kohastructure.sql @@ +1723,4 @@ > `title` varchar(250) NOT NULL default '', -- title of the news article > `content` MEDIUMTEXT NOT NULL, -- the body of your news article > `lang` varchar(50) NOT NULL default '', -- location for the article (koha is the staff client, slip is the circulation receipt and language codes are for the opac) > + `publicationdate` date DEFAULT NULL, -- publication date Should not it be published_on? (In reply to Jonathan Druart from comment #35) > Comment on attachment 109192 [details] [review] [review] > Bug 21066: Database revision > > Review of attachment 109192 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: installer/data/mysql/kohastructure.sql > @@ +1723,4 @@ > > `title` varchar(250) NOT NULL default '', -- title of the news article > > `content` MEDIUMTEXT NOT NULL, -- the body of your news article > > `lang` varchar(50) NOT NULL default '', -- location for the article (koha is the staff client, slip is the circulation receipt and language codes are for the opac) > > + `publicationdate` date DEFAULT NULL, -- publication date > > Should not it be published_on? I agree. (In reply to Jonathan Druart from comment #35) > Should not it be published_on? These patches date from 2018. Have gone through QA. I think this tiny change is quite arbitrary and should not block this patch set now. Created attachment 109514 [details] [review] Bug 21066: DBRev 20.06.00.033 Created attachment 109515 [details] [review] Bug 21066: Rename publicationdate with published_on (In reply to Marcel de Rooy from comment #37) > (In reply to Jonathan Druart from comment #35) > > Should not it be published_on? > > These patches date from 2018. Have gone through QA. I think this tiny change > is quite arbitrary and should not block this patch set now. I don't want to block it. Do you agree with the renaming? (In reply to Jonathan Druart from comment #40) > (In reply to Marcel de Rooy from comment #37) > > (In reply to Jonathan Druart from comment #35) > > > Should not it be published_on? > > > > These patches date from 2018. Have gone through QA. I think this tiny change > > is quite arbitrary and should not block this patch set now. > > I don't want to block it. Do you agree with the renaming? Yes, no problem glancing thru the patch diagonally.. Pushed to master for 20.11, thanks to everybody involved! (In reply to Jonathan Druart from comment #42) > Pushed to master for 20.11, thanks to everybody involved! Cool enhancement will not be backported to 20.05.x *** Bug 23430 has been marked as a duplicate of this bug. *** |