Description
Jonathan Druart
2016-02-17 11:39:03 UTC
Created attachment 48166 [details] [review] Bug 15839: Koha::Reviews - Add Koha::Review[s] classes The subroutines in C4::Reviews are only doing CRUD operations. This patch set moves them to 2 new Koha::Review[s] classes. Test plan for the whole patch set: 0/ Set the reviewson syspref 1/ At the OPAC, create a couple of reviews for a record Try and create a review without html tags different that br b i em big small strong: You should get a warning. 2/ Edit a review 3/ On the Staff interface, you should see a "Comments pending approval" link on the mainpage 4/ Approve 1 review and delete the other 5/ At the OPAC you should only see 1 review (the approved one) in the "Comments" tab 6/ Modify the review 7/ The review should appear again in the "comments awaiting moderation" tab. Note that even the comment has not been changed, it will have to be reapproved (FIXME later). This behavior already existed prior to this patch. Created attachment 48167 [details] [review] Bug 15839: Koha::Reviews - Remove getreviews The C4::Reviews::getreviews subroutine retrieved the reviews for a given record, depending on their status. This can be achieve with a call to Koha::Reviews->search. There were 2 calls to this subroutine. The one from opac-ISBDdetail.pl does not look in used: the reviews are not display on this page. It certainly comes from an old copy/paste from opac-detail.pl. The one from opac-detail only asked for the approved reviews. So the logged in user does not see its own review if it is pending approval. Actually this pending approval review is only displayed when the user submits it (because of a unecessary complex text replacement done in JS). With this patch, the approved reviews AND the unaproved review from the logged in user will be displayed. It will allow a future enhancement to add a way to delete our own reviews. Moreover, the reviews were retrieved even if they were not displayed (if reviewson is off), it's now fixed. Created attachment 48168 [details] [review] Bug 15839: Koha::Reviews - Remove getallreviews Created attachment 48169 [details] [review] Bug 15839: Koha::Reviews - Remove approvereview & unapprovereview This patch adds 2 new methods to Koha::Review: approve and unapprove. Created attachment 48170 [details] [review] Bug 15839: Koha::Reviews - Change default value for approved to 0 The reviews.approved column had a default value set to NULL. It does not make sense, the default value should be 0, this will avoid to have to specify the approved value when creating a new review. Created attachment 48171 [details] [review] Bug 15839: Koha::Reviews - Remove deletereview Note that a future enhancement should take into account the possible error returned by ->approve, ->unapprove and ->delete Created attachment 48172 [details] [review] Bug 15839: Koha::Reviews - Remove getreview Note that this (biblionumber, borrowernumber) pair should be defined as a unique key at the DB level (FIXME added) Created attachment 48173 [details] [review] Bug 15839: Koha::Reviews - Remove getnumberofreviews Created attachment 48174 [details] [review] Bug 15839: Koha::Reviews - Remove updatereview Created attachment 48175 [details] [review] Bug 15839: Koha::Reviews - Remove savereview Created attachment 48176 [details] [review] Bug 15839: Koha::Reviews - Remove C4::Review residue Created attachment 48280 [details] [review] Bug 15839: Koha::Reviews - Add Koha::Review[s] classes The subroutines in C4::Reviews are only doing CRUD operations. This patch set moves them to 2 new Koha::Review[s] classes. Test plan for the whole patch set: 0/ Set the reviewson syspref 1/ At the OPAC, create a couple of reviews for a record Try and create a review without html tags different that br b i em big small strong: You should get a warning. 2/ Edit a review 3/ On the Staff interface, you should see a "Comments pending approval" link on the mainpage 4/ Approve 1 review and delete the other 5/ At the OPAC you should only see 1 review (the approved one) in the "Comments" tab 6/ Modify the review 7/ The review should appear again in the "comments awaiting moderation" tab. Note that even the comment has not been changed, it will have to be reapproved (FIXME later). This behavior already existed prior to this patch. Tested all patches together (with database updated) Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48281 [details] [review] Bug 15839: Koha::Reviews - Remove getreviews The C4::Reviews::getreviews subroutine retrieved the reviews for a given record, depending on their status. This can be achieve with a call to Koha::Reviews->search. There were 2 calls to this subroutine. The one from opac-ISBDdetail.pl does not look in used: the reviews are not display on this page. It certainly comes from an old copy/paste from opac-detail.pl. The one from opac-detail only asked for the approved reviews. So the logged in user does not see its own review if it is pending approval. Actually this pending approval review is only displayed when the user submits it (because of a unecessary complex text replacement done in JS). With this patch, the approved reviews AND the unaproved review from the logged in user will be displayed. It will allow a future enhancement to add a way to delete our own reviews. Moreover, the reviews were retrieved even if they were not displayed (if reviewson is off), it's now fixed. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48282 [details] [review] Bug 15839: Koha::Reviews - Remove getallreviews Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48283 [details] [review] Bug 15839: Koha::Reviews - Remove approvereview & unapprovereview This patch adds 2 new methods to Koha::Review: approve and unapprove. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48284 [details] [review] Bug 15839: Koha::Reviews - Change default value for approved to 0 The reviews.approved column had a default value set to NULL. It does not make sense, the default value should be 0, this will avoid to have to specify the approved value when creating a new review. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48285 [details] [review] Bug 15839: Koha::Reviews - Remove deletereview Note that a future enhancement should take into account the possible error returned by ->approve, ->unapprove and ->delete Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48286 [details] [review] Bug 15839: Koha::Reviews - Remove getreview Note that this (biblionumber, borrowernumber) pair should be defined as a unique key at the DB level (FIXME added) Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48287 [details] [review] Bug 15839: Koha::Reviews - Remove getnumberofreviews Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48288 [details] [review] Bug 15839: Koha::Reviews - Remove updatereview Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48289 [details] [review] Bug 15839: Koha::Reviews - Remove savereview Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 48290 [details] [review] Bug 15839: Koha::Reviews - Remove C4::Review residue Signed-off-by: Marc Véron <veron@veron.ch> Please rebase! Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 15839: Koha::Reviews - Add Koha::Review[s] classes Applying: Bug 15839: Koha::Reviews - Remove getreviews Applying: Bug 15839: Koha::Reviews - Remove getallreviews Applying: Bug 15839: Koha::Reviews - Remove approvereview & unapprovereview Applying: Bug 15839: Koha::Reviews - Change default value for approved to 0 Applying: Bug 15839: Koha::Reviews - Remove deletereview fatal: sha1 information is lacking or useless (C4/Review.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 15839: Koha::Reviews - Remove deletereview The copy of the patch that failed is found in: /home/katrin/kohaclone/.git/rebase-apply/patch Created attachment 51197 [details] [review] Bug 15839: Koha::Reviews - Add Koha::Review[s] classes The subroutines in C4::Reviews are only doing CRUD operations. This patch set moves them to 2 new Koha::Review[s] classes. Test plan for the whole patch set: 0/ Set the reviewson syspref 1/ At the OPAC, create a couple of reviews for a record Try and create a review without html tags different that br b i em big small strong: You should get a warning. 2/ Edit a review 3/ On the Staff interface, you should see a "Comments pending approval" link on the mainpage 4/ Approve 1 review and delete the other 5/ At the OPAC you should only see 1 review (the approved one) in the "Comments" tab 6/ Modify the review 7/ The review should appear again in the "comments awaiting moderation" tab. Note that even the comment has not been changed, it will have to be reapproved (FIXME later). This behavior already existed prior to this patch. Tested all patches together (with database updated) Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51198 [details] [review] Bug 15839: Koha::Reviews - Remove getreviews The C4::Reviews::getreviews subroutine retrieved the reviews for a given record, depending on their status. This can be achieve with a call to Koha::Reviews->search. There were 2 calls to this subroutine. The one from opac-ISBDdetail.pl does not look in used: the reviews are not display on this page. It certainly comes from an old copy/paste from opac-detail.pl. The one from opac-detail only asked for the approved reviews. So the logged in user does not see its own review if it is pending approval. Actually this pending approval review is only displayed when the user submits it (because of a unecessary complex text replacement done in JS). With this patch, the approved reviews AND the unaproved review from the logged in user will be displayed. It will allow a future enhancement to add a way to delete our own reviews. Moreover, the reviews were retrieved even if they were not displayed (if reviewson is off), it's now fixed. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51199 [details] [review] Bug 15839: Koha::Reviews - Remove getallreviews Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51200 [details] [review] Bug 15839: Koha::Reviews - Remove approvereview & unapprovereview This patch adds 2 new methods to Koha::Review: approve and unapprove. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51201 [details] [review] Bug 15839: Koha::Reviews - Change default value for approved to 0 The reviews.approved column had a default value set to NULL. It does not make sense, the default value should be 0, this will avoid to have to specify the approved value when creating a new review. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51202 [details] [review] Bug 15839: Koha::Reviews - Remove deletereview Note that a future enhancement should take into account the possible error returned by ->approve, ->unapprove and ->delete Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51203 [details] [review] Bug 15839: Koha::Reviews - Remove getreview Note that this (biblionumber, borrowernumber) pair should be defined as a unique key at the DB level (FIXME added) Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51204 [details] [review] Bug 15839: Koha::Reviews - Remove getnumberofreviews Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51205 [details] [review] Bug 15839: Koha::Reviews - Remove updatereview Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51206 [details] [review] Bug 15839: Koha::Reviews - Remove savereview Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 51207 [details] [review] Bug 15839: Koha::Reviews - Remove C4::Review residue Signed-off-by: Marc Véron <veron@veron.ch> QA: Looking here now.. qa tools: Processing files after patches fatal: ambiguous argument 'C4/Review.pm': unknown revision or path not in the working tree. Use '--' to separate paths from revisions QA Comment: If I submit a new review, the approved status will be NULL. But reviewswaiting.pl will not show it: my $status = $query->param('status') || 0; ... my $reviews = Koha::Reviews->search( { approved => $status }, (In reply to Marcel de Rooy from comment #37) > QA Comment: > If I submit a new review, the approved status will be NULL. > But reviewswaiting.pl will not show it: > > my $status = $query->param('status') || 0; > ... > my $reviews = Koha::Reviews->search( > { approved => $status }, Marcel, This is fixed by "Change default value for approved to 0", please execute the updatedatabase.pl and update the schema. (In reply to Jonathan Druart from comment #38) > (In reply to Marcel de Rooy from comment #37) > > QA Comment: > > If I submit a new review, the approved status will be NULL. > > But reviewswaiting.pl will not show it: > > > > my $status = $query->param('status') || 0; > > ... > > my $reviews = Koha::Reviews->search( > > { approved => $status }, > > Marcel, > This is fixed by "Change default value for approved to 0", please execute > the updatedatabase.pl and update the schema. QA: Looking again Marc, The é in Véron still makes me crazy. Especially in 11 patches :) Created attachment 53794 [details] [review] Bug 15839: Koha::Reviews - Add Koha::Review[s] classes The subroutines in C4::Reviews are only doing CRUD operations. This patch set moves them to 2 new Koha::Review[s] classes. Test plan for the whole patch set: 0/ Set the reviewson syspref 1/ At the OPAC, create a couple of reviews for a record Try and create a review without html tags different that br b i em big small strong: You should get a warning. 2/ Edit a review 3/ On the Staff interface, you should see a "Comments pending approval" link on the mainpage 4/ Approve 1 review and delete the other 5/ At the OPAC you should only see 1 review (the approved one) in the "Comments" tab 6/ Modify the review 7/ The review should appear again in the "comments awaiting moderation" tab. Note that even the comment has not been changed, it will have to be reapproved (FIXME later). This behavior already existed prior to this patch. Tested all patches together (with database updated) Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53795 [details] [review] Bug 15839: Koha::Reviews - Remove getreviews The C4::Reviews::getreviews subroutine retrieved the reviews for a given record, depending on their status. This can be achieve with a call to Koha::Reviews->search. There were 2 calls to this subroutine. The one from opac-ISBDdetail.pl does not look in used: the reviews are not display on this page. It certainly comes from an old copy/paste from opac-detail.pl. The one from opac-detail only asked for the approved reviews. So the logged in user does not see its own review if it is pending approval. Actually this pending approval review is only displayed when the user submits it (because of a unecessary complex text replacement done in JS). With this patch, the approved reviews AND the unaproved review from the logged in user will be displayed. It will allow a future enhancement to add a way to delete our own reviews. Moreover, the reviews were retrieved even if they were not displayed (if reviewson is off), it's now fixed. Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53796 [details] [review] Bug 15839: Koha::Reviews - Remove getallreviews Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53797 [details] [review] Bug 15839: Koha::Reviews - Remove approvereview & unapprovereview This patch adds 2 new methods to Koha::Review: approve and unapprove. Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53798 [details] [review] Bug 15839: Koha::Reviews - Change default value for approved to 0 The reviews.approved column had a default value set to NULL. It does not make sense, the default value should be 0, this will avoid to have to specify the approved value when creating a new review. Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53799 [details] [review] Bug 15839: Koha::Reviews - Remove deletereview Note that a future enhancement should take into account the possible error returned by ->approve, ->unapprove and ->delete Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53800 [details] [review] Bug 15839: Koha::Reviews - Remove getreview Note that this (biblionumber, borrowernumber) pair should be defined as a unique key at the DB level (FIXME added) Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53801 [details] [review] Bug 15839: Koha::Reviews - Remove getnumberofreviews Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53802 [details] [review] Bug 15839: Koha::Reviews - Remove updatereview Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53803 [details] [review] Bug 15839: Koha::Reviews - Remove savereview Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53804 [details] [review] Bug 15839: Koha::Reviews - Remove C4::Review residue Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 53805 [details] [review] Bug 15839: [QA Follow-up] Update existing rows in dbrev Adding the default is fine for new records; we also should take care of the pending existing ones. Trivial addition to atomic update file. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested the dbrev again with existing unapproved reviews. Created attachment 53806 [details] [review] Bug 15839: [QA Follow-up] Paging on opac-showreviews Paging is kind of messy here. This patch at least improves somewhat. The page number should be rounded. The results per page should be passed to the template too. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested a number of reviews and played with count parameter in URL. Created attachment 53807 [details] [review] Bug 15839: [QA Follow-up] Error checking in opac-review.pl [1] Adds a check on biblionumber. (Prevents a DBIx error.) [2] If you have a reviewid, search on that and check results. Add an unauthorized error in template. [3] If you add a new review, check that there is no review yet. If so, edit the existing one. This supports the added FIXME on a unique constraint. Note: This script could receive further attention. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested all crud ops with opac-review.pl (incl URL manipulation). qa tools: OK t/db_dependent/Koha/Reviews.t OK Added a follow-up for db rev Loop in opac-detail repeatedly calls GetMember for possibly same borrower? Enabled ShowRecentComments opac-showreviews: Improved paging in a small follow-up. (The count parameter is not kept when paging.) reviewswaiting: When I delete an approved review, the display jumps back to awaiting moderation. Just to mention: If I have two comments on a book, these two icons are weird imo. (Even one, but that is personal taste. :) opac-review: See follow-up Passed QA Pushed to master for 16.11, thanks Jonathan, Marcel! |