Description
Marcel de Rooy
2012-11-08 13:59:38 UTC
(In reply to comment #0) > The owner can 'unshare' the list and remove all shares for that list. I am having second thoughts about this part of the functionality. I would rather opt now for copy and delete without unsharing. Created attachment 18260 [details] [review] Bug 9032: Share a list: OPAC foundation Adds a Share button for OPAC private lists Adds template and script for further development Test plan: Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. Please wait for some more patches :) Created attachment 18542 [details] [review] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies hash. Test plan: You should see it now under About. Created attachment 18543 [details] [review] Bug 9032: Share a list (dbrev for SHARE_INVITE notice) Adds notice in updatedatabase and sample_notices. Test plan: Do install or upgrade and verify that you have the new notice under Tools. Comment from Robin on dev list: On this note, it's also best to use modules that are packaged in Debian squeeze (which I figure we'll try to support as long as we can) where possible, just to save me the work in making packages for them :) Also, cpan modules that have no license that allows us to redistribute them are a no-no. This doesn't apply to libemail-valid-perl however, it's in squeeze already. Created attachment 18546 [details] [review] Bug 9032: Share a list (dbrev for SHARE_INVITE notice) Adds notice in updatedatabase and sample_notices. Test plan: Do install or upgrade and verify that you have the new notice under Tools. Created attachment 18547 [details] [review] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies module. Note from Robin: libemail-valid-perl is in squeeze already. Test plan: You should see Email::Valid now under About/Perl modules. If Email::Valid is not yet installed on your system, please do so. Created attachment 18587 [details] [review] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). NOTE: This patch adds a routine to VirtualShelves.pm but does not add a test in VirtualShelves.t. This is intentional: I will still add a test, but saw report 10290 and came across a few other problems in this unit test. Will add a patch under report 10386. Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. As part of ongoing development, these three patches are imho ready for at least signoff. They should not be pushed without the followup on bug 10389. This allows for gradual test and qa on smaller patches. I would also welcome followups on getting the text but especially the design on a higher level; I am no expert in that area :) Thanks. Created attachment 18619 [details] [review] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). NOTE: This patch adds a routine to VirtualShelves.pm but does not add a test in VirtualShelves.t. This is intentional: I will still add a test, but saw report 10290 and came across a few other problems in this unit test. Will add a patch under report 10386. Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. Small adjustment: one line for debugging purposes removed. Adds Share button on view private shelf form. Created attachment 18682 [details] [review] Bug 9032 [Template follow-up] Share a list (part 1: send the invitation) This patch makes a few changes to the "share a list" template: - We don't use tables for layout of forms. This patch corrects the display of the share form to use Koha standard form markup. - Revise the confirmation message displayed after a list is shared to be more readable. - Add a link back to private lists to the confirmation message. - Add T:T markup to make this page display the left-hand navigation which is shown on other patron-related pages when a user is logged in. To test, apply this patch on top of other patches pending for Bug 9032 and confirm that the list share form looks correct and works correctly. Confirm that share confirmation page reads well. I see now that you can enter multiple email addresses when sharing. Text should be added to make that clear, including specifying what separator is required (a comma works, what about something else?). How will the sharing feature handle a case where two patrons have the same email address? We find that this happens with families with children--the parent will put their email address on the records of each of their children. Created attachment 18692 [details] [review] [SIGNED_OFF] Bug 9032 [Template follow-up] Share a list (part 1: send the invitation) This patch makes a few changes to the "share a list" template: - We don't use tables for layout of forms. This patch corrects the display of the share form to use Koha standard form markup. - Revise the confirmation message displayed after a list is shared to be more readable. - Add a link back to private lists to the confirmation message. - Add T:T markup to make this page display the left-hand navigation which is shown on other patron-related pages when a user is logged in. To test, apply this patch on top of other patches pending for Bug 9032 and confirm that the list share form looks correct and works correctly. Confirm that share confirmation page reads well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to comment #14) > I see now that you can enter multiple email addresses when sharing. Text > should be added to make that clear, including specifying what separator is > required (a comma works, what about something else?). > > How will the sharing feature handle a case where two patrons have the same > email address? We find that this happens with families with children--the > parent will put their email address on the records of each of their children. Hi Owen, Thanks for testing and followup. What about one line under the form like: You can enter multiple email addresses, if you separate them by semicolons. Note: I used [,;:] in the code, but was having doubts since some of these chars actually are allowed in an email address when placed between quotes. The semicolon seems to be used more often as separator between mail addresses. The share feature uses the email addresses given. If an address is repeated, two mails will be sent. The address used does not need to be the patron email address. Crucial in the acceptance is that you confirm with your login. Seems more flexible and 'secure'. The children in your example just need to accept with their own login. Created attachment 19377 [details] [review] [SIGNED-OFF] Bug 9032: Share a list (dbrev for SHARE_INVITE notice) Adds notice in updatedatabase and sample_notices. Test plan: Do install or upgrade and verify that you have the new notice under Tools. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 19378 [details] [review] [SIGNED-OFF] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies module. Note from Robin: libemail-valid-perl is in squeeze already. Test plan: You should see Email::Valid now under About/Perl modules. If Email::Valid is not yet installed on your system, please do so. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 19379 [details] [review] [SIGNED-OFF] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). NOTE: This patch adds a routine to VirtualShelves.pm but does not add a test in VirtualShelves.t. This is intentional: I will still add a test, but saw report 10290 and came across a few other problems in this unit test. Will add a patch under report 10386. Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Note to QA: Because of the order in which the patches were signed off the first one, "[Template follow-up] Share a list (part 1: send the invitation)" should be applied last. Created attachment 19394 [details] [review] [SIGNED-OFF] Bug 9032 [Template follow-up] Share a list (part 1: send the invitation) This patch makes a few changes to the "share a list" template: - We don't use tables for layout of forms. This patch corrects the display of the share form to use Koha standard form markup. - Revise the confirmation message displayed after a list is shared to be more readable. - Add a link back to private lists to the confirmation message. - Add T:T markup to make this page display the left-hand navigation which is shown on other patron-related pages when a user is logged in. To test, apply this patch on top of other patches pending for Bug 9032 and confirm that the list share form looks correct and works correctly. Confirm that share confirmation page reads well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Owen Leonard from comment #20) > Note to QA: Because of the order in which the patches were signed off the > first one, "[Template follow-up] Share a list (part 1: send the invitation)" > should be applied last. I have rearranged the order. Thanks Owen for testing. QA: These patches are ready for QA, separate from the followup reports. They can stay in the QA queue, if they pass, until at least part2 comes along (bug 10389). Will submit patches for that soon. Part 3 will handle the orphaned lists and permissions for taking ownership, moderating public/shared list names. Created attachment 19934 [details] [review] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). NOTE: This patch adds a routine to VirtualShelves.pm but does not add a test in VirtualShelves.t. This is intentional: I will still add a test, but saw report 10290 and came across a few other problems in this unit test. Will add a patch under report 10386. Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 19935 [details] [review] Bug 9032 [Template follow-up] Share a list (part 1: send the invitation) This patch makes a few changes to the "share a list" template: - We don't use tables for layout of forms. This patch corrects the display of the share form to use Koha standard form markup. - Revise the confirmation message displayed after a list is shared to be more readable. - Add a link back to private lists to the confirmation message. - Add T:T markup to make this page display the left-hand navigation which is shown on other patron-related pages when a user is logged in. To test, apply this patch on top of other patches pending for Bug 9032 and confirm that the list share form looks correct and works correctly. Confirm that share confirmation page reads well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased the last two patches. QA comment: The code: - Perltidies the new file please. - Use the CPLv3 license: FAIL opac/opac-shareshelf.pl FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 9) - The new letter should be added to others languages. - The new pl file contains a lot of routines. Some of them use global variables which are not defined as global. - Maybe SHARE_INVITATION_EXPIRY_DAYS should be a new syspref? Hum... or not. - Note that ADDDATE is a mysqlism, but Koha doesn't offer a better way to do. The ergonomic: - The page opac-shelves.pl?display=privateshelves displays a link for editing and deleting, but a button for sharing. Is it intentional? - There is no back button if I enter a bad email address. The feature: The invitation is sent to the message queue. The doc suggests process_message_queue.pl runs every 1 to 4 hours. We can imagine that a user tells to his friend: "I just share a list with you. You will receive an email!". The friend refresh, refresh, refresh his inbox and nothing append. The guy will resent the invitation again and again :) I have 2 suggestions: * directly use the sendmail command. * Add a check: if an invitation for this list and this email address has been sent some time ago, an alert is raised and no new invitation is sent. Marked as Failed QA. (In reply to Jonathan Druart from comment #26) > The invitation is sent to the message queue. The doc suggests > process_message_queue.pl runs every 1 to 4 hours. We can imagine that a user > tells to his friend: "I just share a list with you. You will receive an > email!". The friend refresh, refresh, refresh his inbox and nothing append. > The guy will resent the invitation again and again :) > I have 2 suggestions: > * directly use the sendmail command. I disagree with this. There is nothing preventing process_message_queue.pl from being run more frequently; in fact, the crontab that goes with the Debian package runs it every 15 minutes -- and it would be reasonable for a library to run it more frequently. I would be loath to give up the centralization and templating options that the message queue gives us. I see that crontab.example does need to be updated, though: it runs the message queue every hour, and it probably wouldn't hurt to bump up the frequencye. > * Add a check: if an invitation for this list and this email address has > been sent some time ago, an alert is raised and no new invitation is sent. This would be reasonable. (In reply to Galen Charlton from comment #27) > I disagree with this. There is nothing preventing process_message_queue.pl > from being run more frequently; This is the same issue that came up in discussions of Bug 7067 - allow patron self registration via the opac. If your patrons are registering through the OPAC and expecting a confirmation email you would want process_message_queue.pl to run as often as once a minute. The snag is that then your message queue "digest" options don't work because the digest never has the time to accumulate messages. (In reply to Owen Leonard from comment #28) > (In reply to Galen Charlton from comment #27) > > I disagree with this. There is nothing preventing process_message_queue.pl > > from being run more frequently; > > This is the same issue that came up in discussions of Bug 7067 - allow > patron self registration via the opac. If your patrons are registering > through the OPAC and expecting a confirmation email you would want > process_message_queue.pl to run as often as once a minute. The snag is that > then your message queue "digest" options don't work because the digest never > has the time to accumulate messages. Fair point. One possibility would be to allow certain message types be marked as high-priority (which would include the interpretation that they're not meant to ever be digested), then add a switch to process_message_queue.pl to have it process just the high-priority ones. I'm willing to put a bit of time into that if folks are amenable to this approach. I think the only messages digested like Owen describes are the checkin and checkout slips - so maybe we could turn it around and have a setting to only send those out every x minutes? I think the other mails could be sent out pretty immediately after being generated without problems. (In reply to Jonathan Druart from comment #26) First of all, thanks for your QA, although the results were negative :) > - The new letter should be added to others languages. I would really get rid of that restriction. I started some development on that (look for i18n). But unfortunately there is not that much support for it. > - The new pl file contains a lot of routines. Some of them use global > variables which are not defined as global. Please explain. > Marked as Failed QA. If this should really fail for the reasons given, I will have to let this development rest for some time. I do not have the time to keep everything in the air now ;) Note that this impacts also unit tests and the accept a share-followup reports. (In reply to M. de Rooy from comment #31) > (In reply to Jonathan Druart from comment #26) > First of all, thanks for your QA, although the results were negative :) > > > - The new letter should be added to others languages. > I would really get rid of that restriction. I started some development on > that (look for i18n). But unfortunately there is not that much support for > it. So should this patch depends on them? > > - The new pl file contains a lot of routines. Some of them use global > > variables which are not defined as global. > Please explain. If you use a global variable in a routine, you should declare it as 'our' instead of 'my'. > > Marked as Failed QA. > If this should really fail for the reasons given, I will have to let this > development rest for some time. I do not have the time to keep everything in > the air now ;) Note that this impacts also unit tests and the accept a > share-followup reports. (In reply to Jonathan Druart from comment #32) > If you use a global variable in a routine, you should declare it as 'our' > instead of 'my'. These variables are only used in some subroutines of this script, eliminating the need for passing them as parameters. I do not need them to be (global) package variables. A lexical variable declared with my is sufficient here. (In reply to M. de Rooy from comment #33) > (In reply to Jonathan Druart from comment #32) > > If you use a global variable in a routine, you should declare it as 'our' > > instead of 'my'. > These variables are only used in some subroutines of this script, > eliminating the need for passing them as parameters. I do not need them to > be (global) package variables. A lexical variable declared with my is > sufficient here. That does not work with Plack. Moreover, using global variables is not a good practice. With Plack, on opac-shareshelf.pl?shelfnumber=1, I got an error 500 (internal server error) with the following in logs: Variable "$op" is not available at /home/koha/src/opac/opac-shareshelf.pl line 70. Variable "$errcode" is not available at /home/koha/src/opac/opac-shareshelf.pl line 71. Variable "$shelfnumber" is not available at /home/koha/src/opac/opac-shareshelf.pl line 74. Variable "@addr" is not available at /home/koha/src/opac/opac-shareshelf.pl line 91. Variable "$errcode" is not available at /home/koha/src/opac/opac-shareshelf.pl line 95. Variable "$addrlist" is not available at /home/koha/src/opac/opac-shareshelf.pl line 104. Variable "$fail_addr" is not available at /home/koha/src/opac/opac-shareshelf.pl line 105. Variable "@addr" is not available at /home/koha/src/opac/opac-shareshelf.pl line 110. Variable "$shelfnumber" is not available at /home/koha/src/opac/opac-shareshelf.pl line 121. Variable "@addr" is not available at /home/koha/src/opac/opac-shareshelf.pl line 125. Variable "@newkey" is not available at /home/koha/src/opac/opac-shareshelf.pl line 126. Variable "$loggedinuser" is not available at /home/koha/src/opac/opac-shareshelf.pl line 133. Variable "$shelfname" is not available at /home/koha/src/opac/opac-shareshelf.pl line 135. Variable "$shelfname" is not available at /home/koha/src/opac/opac-shareshelf.pl line 154. Variable "$owner" is not available at /home/koha/src/opac/opac-shareshelf.pl line 154. Variable "$shelfnumber" is not available at /home/koha/src/opac/opac-shareshelf.pl line 154. Variable "$errcode" is not available at /home/koha/src/opac/opac-shareshelf.pl line 155. Variable "$loggedinuser" is not available at /home/koha/src/opac/opac-shareshelf.pl line 155. Variable "$template" is not available at /home/koha/src/opac/opac-shareshelf.pl line 162. Variable "$loggedinuser" is not available at /home/koha/src/opac/opac-shareshelf.pl line 162. Variable "$cookie" is not available at /home/koha/src/opac/opac-shareshelf.pl line 162. Variable "$query" is not available at /home/koha/src/opac/opac-shareshelf.pl line 164. Variable "$template" is not available at /home/koha/src/opac/opac-shareshelf.pl line 171. Variable "$errcode" is not available at /home/koha/src/opac/opac-shareshelf.pl line 172. Variable "$op" is not available at /home/koha/src/opac/opac-shareshelf.pl line 173. Variable "$shelfnumber" is not available at /home/koha/src/opac/opac-shareshelf.pl line 174. Variable "$shelfname" is not available at /home/koha/src/opac/opac-shareshelf.pl line 175. Variable "@addr" is not available at /home/koha/src/opac/opac-shareshelf.pl line 176. Variable "$fail_addr" is not available at /home/koha/src/opac/opac-shareshelf.pl line 177. Variable "@base64alphabet" is not available at /home/koha/src/opac/opac-shareshelf.pl line 225. no query in gettemplate at /home/koha/src/C4/Templates.pm line 232. no query in themelanguage at /home/koha/src/C4/Templates.pm line 282. Can't call method "cookie" on an undefined value at /home/koha/src/C4/Templates.pm line 331. (In reply to Jonathan Druart from comment #34) > That does not work with Plack. > Moreover, using global variables is not a good practice. Thanks. I was somewhat expecting to hear Plack coming :) We should add this to the coding guidelines. It is not there yet. For the record, I did not use global variables (my variables are lexical). Do you have a reference to a best practice or so about not using lexical variables on 'root level'? This is not in the coding guidelines too? Jonathan, You convinced me for the Plack reason. But your other argument? Note that PBP on the opposite tends to move to using lexical variables instead of global package vars (where possible). So we should not use our if we can solve it differently. In this case for example, i could pass lower my variables via parameters.. (In reply to M. de Rooy from comment #37) > Jonathan, > You convinced me for the Plack reason. > > But your other argument? > Note that PBP on the opposite tends to move to using lexical variables > instead of global package vars (where possible). So we should not use our if > we can solve it differently. In this case for example, i could pass lower my > variables via parameters.. Hello Marcel, It is a vocabulary issue :) What I wrongly called a global variable was a variable declared outside of the routine where it is used. Obviously, passing the variables in parameter is better than declaring them as global (our). Hope to revive this development next week by rebasing, making the requested QA changes and including work from the former dependency bug 10389 and bug 10441. Created attachment 25847 [details] [review] Bug 9032: Share a list (database revision for notices) Adds notice SHARE_INVITE in updatedatabase and sample_notices. [Amendation: also adds SHARE_ACCEPT notice.] Test plan: Install or upgrade and verify that you have the new notice under Tools. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Amended the patch to add the second notice SHARE_ACCEPT. Tested upgrade with updatestructure step of web installer. Tested install by deleting all notices and running sample_notices.sql NOTE: Will add the notices to the other sql files in the other language directories as soon as this set of patches is ready to be pushed in order to reduce the need to rebase them constantly. Created attachment 25848 [details] [review] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies module. Note from Robin: libemail-valid-perl is in squeeze already. Test plan: You should see Email::Valid now under About/Perl modules. If Email::Valid is not yet installed on your system, please do so. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Created attachment 25849 [details] [review] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased. Small amendation: rewording on email notification in template. Created attachment 25850 [details] [review] Bug 9032 [Template follow-up] Share a list (part 1: send the invitation) This patch makes a few changes to the "share a list" template: - We don't use tables for layout of forms. This patch corrects the display of the share form to use Koha standard form markup. - Revise the confirmation message displayed after a list is shared to be more readable. - Add a link back to private lists to the confirmation message. - Add T:T markup to make this page display the left-hand navigation which is shown on other patron-related pages when a user is logged in. To test, apply this patch on top of other patches pending for Bug 9032 and confirm that the list share form looks correct and works correctly. Confirm that share confirmation page reads well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased March 5, 2014. Created attachment 25851 [details] [review] Bug 9032: [QA Follow-up] Plackify opac-shareshelf.pl Plackified opac-shareshelf by moving global my variables into hashref. Slightly changed base64ord and base64chr functions to more generic ones. Test plan: Verify if sending an share invitation still works (under prog theme). Created attachment 25852 [details] [review] Bug 9032: Share a list (accept invitation and remove a share) This patch handles: [1] The response (acceptance) by the invited person. If he accepts this share, the private list of the sender will be shown under Your lists on the shelves page. In OPAC 'Your private lists' has been renamed to Your lists (just as in Staff). The Type column shows Private or Shared for these lists; a list appears as Shared as soon as an invitation has been accepted. The owner has the options to Edit, Delete or Share; the invited person does not have these options on the shared list. [2] Removing an accepted share. If a user accepted a share, he should also be able to remove it again. The Remove Share button is visible on OPAC when viewing Your lists or a particular shared list. Note: AddShare has been extended to return a possible database error. If the share invite could not be added, a mail will not be sent. Test plan (for prog theme): Enable pref OpacAllowSharingPrivateLists User 1 creates new private list P1, perms: D-A-D, adds 2 items, sends share User 1 checks your lists display: is P1 Private with Edit button? User 2 accepts share: sees P1, but cannot add or delete items User 2 checks your lists display again: P1 shows Shared without Edit? User 1 checks your lists display again: P1 shows Shared with Edit? User 2 tries to accept share again: should fail now User 3 tries to accept share: should also fail User 3 tries again, modifies shelfnumber and/or key in url: should also fail User 2 creates new private list P2, perms: A-A-A, no items, sends share User 2 checks your lists display: P2 shows Private with Edit? User 1 accepts, adds one item User 1 checks your lists display: P2 shows Shared without Edit? User 2 checks your lists display: P2 shows Shared with Edit? User 2 deletes item of user 1 (allowed) User 2 deletes list P2 User 1 checks your lists display in opac or staff: P2 is gone? User 1 creates private list P3, sends a share. User 1 creates private list P4, adds one item, sends a share. User 2 accepts the share for P3. User 2 checks the shelves display, and removes share P3. User 2 accepts the share for P4. User 2 views shelf P4 with one item and confirms Remove share on that form. User 2 checks shelves display again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Combined changes for AcceptShare and RemoveShare. Created attachment 25853 [details] [review] Bug 9032: Unit tests for sharing lists This patch combines some work from report 10389 and 10441. With thanks to Srdjan and Jonathan Druart for their contributions. Adds tests for AddShare, AcceptShare, IsSharedList, RemoveShare. This patch also restores the inadvertently deleted DelShelf tests in bug 10499. Test plan: Run the test VirtualShelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased and squashed on March 4, 2013. Before changing the status, I will still add at lease one patch. This is to handle the bootstrap theme. Still thinking about one additional point too.. Created attachment 25862 [details] [review] Bug 9032: Consistent behavior when deleting lists DelShelf deletes a list regardless whether it is private, shared or public. HandleDelBorrower had another approach, trying to save shared and public lists by setting the owner to NULL. This patch makes both routines behave consistent. A new report 11889 has been opened to discuss the 'disowning' feature. Test plan: Create a list P1 with user1 that allows adding by other users. Add a patron (user2). Login as user2 and create some lists, add some items. Let user2 add some entries to P1 too. Delete patron user2. Verify that his lists are gone, but his entries in P1 are kept (nullified). Created attachment 25886 [details] [review] Bug 9032: Consistent behavior when deleting lists DelShelf deletes a list regardless whether it is private, shared or public. HandleDelBorrower had another approach, trying to save shared and public lists by setting the owner to NULL. This patch makes both routines behave consistent. A new report 11889 has been opened to discuss the 'disowning' feature. NOTE: I did not add a db revision here to handle possible cases of lists without owner in the current data. Such public (or shared) lists can still be used without any problem. Bug 11889 and a new planned report for a lists management tool will address this topic further on. After that, all goals of umbrella report 7310 should be realized. Test plan: Create a list P1 with user1 that allows adding by other users. Add a patron (user2). Login as user2 and create some lists, add some items. Let user2 add some entries to P1 too. Delete patron user2. Verify that his lists are gone, but his entries in P1 are kept (nullified). Created attachment 25893 [details] [review] Bug 9032: Follow-up for bootstrap theme Adds new template opac-shareshelf.tt. Modifies template opac-shelves.tt: Share button, Remove Share button, label "Your lists" instead of "Your private lists", list category Shared. Test plan: Verify if the Share a list-features work in bootstrap by: [1] Switch to bootstrap. Go to Lists. [2] Share one of your private lists. [3] Login as another user and accept the invitation. [4] Remove the share again. [5] Check if Share and Remove share do not popup for public lists. This is ready for signoff again. Note that some of the patches already had a signoff. Some have been amended while rebasing (as noted). Some patches are new. In other words: Please help to get this further. Thanks! Created attachment 26253 [details] [review] Bug 9032: Share a list (database revision for notices) Adds notice SHARE_INVITE in updatedatabase and sample_notices. [Amendation: also adds SHARE_ACCEPT notice.] Test plan: Install or upgrade and verify that you have the new notice under Tools. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Amended the patch to add the second notice SHARE_ACCEPT. Tested upgrade with updatestructure step of web installer. Tested install by deleting all notices and running sample_notices.sql NOTE: Will add the notices to the other sql files in the other language directories as soon as this set of patches is ready to be pushed in order to reduce the need to rebase them constantly. Trivial rebase of the dbrev for easier signoff :) I was testing this patch and when I copy/paste URL without being logged in into OPAC I get following error: ERROR: You do not have adequate permission for that action on list 1330. However, after that, I'm not logged in into OPAC. If I login after that I indeed see shared list and everything works fine. This might be problem with Koha login mechanism instead of this patch, but it's just interesting to note. If I'm already logged in into OPAC it works as expected. Other than that, this patch provides very useful addition to Koha, thanks. Created attachment 26291 [details] [review] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies module. Note from Robin: libemail-valid-perl is in squeeze already. Test plan: You should see Email::Valid now under About/Perl modules. If Email::Valid is not yet installed on your system, please do so. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26292 [details] [review] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies module. Note from Robin: libemail-valid-perl is in squeeze already. Test plan: You should see Email::Valid now under About/Perl modules. If Email::Valid is not yet installed on your system, please do so. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26293 [details] [review] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased. Small amendation: rewording on email notification in template. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26294 [details] [review] Bug 9032 [Template follow-up] Share a list (part 1: send the invitation) This patch makes a few changes to the "share a list" template: - We don't use tables for layout of forms. This patch corrects the display of the share form to use Koha standard form markup. - Revise the confirmation message displayed after a list is shared to be more readable. - Add a link back to private lists to the confirmation message. - Add T:T markup to make this page display the left-hand navigation which is shown on other patron-related pages when a user is logged in. To test, apply this patch on top of other patches pending for Bug 9032 and confirm that the list share form looks correct and works correctly. Confirm that share confirmation page reads well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased March 5, 2014. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26295 [details] [review] Bug 9032: [QA Follow-up] Plackify opac-shareshelf.pl Plackified opac-shareshelf by moving global my variables into hashref. Slightly changed base64ord and base64chr functions to more generic ones. Test plan: Verify if sending an share invitation still works (under prog theme). Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26296 [details] [review] Bug 9032: Share a list (accept invitation and remove a share) This patch handles: [1] The response (acceptance) by the invited person. If he accepts this share, the private list of the sender will be shown under Your lists on the shelves page. In OPAC 'Your private lists' has been renamed to Your lists (just as in Staff). The Type column shows Private or Shared for these lists; a list appears as Shared as soon as an invitation has been accepted. The owner has the options to Edit, Delete or Share; the invited person does not have these options on the shared list. [2] Removing an accepted share. If a user accepted a share, he should also be able to remove it again. The Remove Share button is visible on OPAC when viewing Your lists or a particular shared list. Note: AddShare has been extended to return a possible database error. If the share invite could not be added, a mail will not be sent. Test plan (for prog theme): Enable pref OpacAllowSharingPrivateLists User 1 creates new private list P1, perms: D-A-D, adds 2 items, sends share User 1 checks your lists display: is P1 Private with Edit button? User 2 accepts share: sees P1, but cannot add or delete items User 2 checks your lists display again: P1 shows Shared without Edit? User 1 checks your lists display again: P1 shows Shared with Edit? User 2 tries to accept share again: should fail now User 3 tries to accept share: should also fail User 3 tries again, modifies shelfnumber and/or key in url: should also fail User 2 creates new private list P2, perms: A-A-A, no items, sends share User 2 checks your lists display: P2 shows Private with Edit? User 1 accepts, adds one item User 1 checks your lists display: P2 shows Shared without Edit? User 2 checks your lists display: P2 shows Shared with Edit? User 2 deletes item of user 1 (allowed) User 2 deletes list P2 User 1 checks your lists display in opac or staff: P2 is gone? User 1 creates private list P3, sends a share. User 1 creates private list P4, adds one item, sends a share. User 2 accepts the share for P3. User 2 checks the shelves display, and removes share P3. User 2 accepts the share for P4. User 2 views shelf P4 with one item and confirms Remove share on that form. User 2 checks shelves display again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Combined changes for AcceptShare and RemoveShare. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26297 [details] [review] Bug 9032: Unit tests for sharing lists This patch combines some work from report 10389 and 10441. With thanks to Srdjan and Jonathan Druart for their contributions. Adds tests for AddShare, AcceptShare, IsSharedList, RemoveShare. This patch also restores the inadvertently deleted DelShelf tests in bug 10499. Test plan: Run the test VirtualShelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased and squashed on March 4, 2013. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26298 [details] [review] Bug 9032: Consistent behavior when deleting lists DelShelf deletes a list regardless whether it is private, shared or public. HandleDelBorrower had another approach, trying to save shared and public lists by setting the owner to NULL. This patch makes both routines behave consistent. A new report 11889 has been opened to discuss the 'disowning' feature. NOTE: I did not add a db revision here to handle possible cases of lists without owner in the current data. Such public (or shared) lists can still be used without any problem. Bug 11889 and a new planned report for a lists management tool will address this topic further on. After that, all goals of umbrella report 7310 should be realized. Test plan: Create a list P1 with user1 that allows adding by other users. Add a patron (user2). Login as user2 and create some lists, add some items. Let user2 add some entries to P1 too. Delete patron user2. Verify that his lists are gone, but his entries in P1 are kept (nullified). Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26299 [details] [review] Bug 9032: Follow-up for bootstrap theme Adds new template opac-shareshelf.tt. Modifies template opac-shelves.tt: Share button, Remove Share button, label "Your lists" instead of "Your private lists", list category Shared. Test plan: Verify if the Share a list-features work in bootstrap by: [1] Switch to bootstrap. Go to Lists. [2] Share one of your private lists. [3] Login as another user and accept the invitation. [4] Remove the share again. [5] Check if Share and Remove share do not popup for public lists. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26300 [details] [review] Bug 9032: Share a list (database revision for notices) Adds notice SHARE_INVITE in updatedatabase and sample_notices. [Amendation: also adds SHARE_ACCEPT notice.] Test plan: Install or upgrade and verify that you have the new notice under Tools. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Amended the patch to add the second notice SHARE_ACCEPT. Tested upgrade with updatestructure step of web installer. Tested install by deleting all notices and running sample_notices.sql NOTE: Will add the notices to the other sql files in the other language directories as soon as this set of patches is ready to be pushed in order to reduce the need to rebase them constantly. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26302 [details] [review] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies module. Note from Robin: libemail-valid-perl is in squeeze already. Test plan: You should see Email::Valid now under About/Perl modules. If Email::Valid is not yet installed on your system, please do so. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Created attachment 26303 [details] [review] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased. Small amendation: rewording on email notification in template. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Some of patches in this searies might change order because of git bz barfing on me when changing bug status to signed off more than once, but thay do apply on top of master as they are ordered now and don't show any diff compared to my version on disk which I signed off. So, sorry if I re-ordered them :-) Created attachment 26351 [details] [review] Bug 9032: QA Follow-up: Send cookie with redirect Small fix to resolve a problem Dobrica noted on the report. If you had to login when accepting the share, the cookie was not passed back to the server when redirecting to shelf display. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Accepted a share while logged in and while not logged in. (In reply to Dobrica Pavlinusic from comment #67) > Some of patches in this searies might change order because of git bz barfing > on me when changing bug status to signed off more than once, but thay do > apply on top of master as they are ordered now and don't show any diff > compared to my version on disk which I signed off. So, sorry if I re-ordered > them :-) No worries. Looks good. Sent an small fix for the login problem. QA comment: I did not check if the current code fixes all QA issues raised on comment 26. Additional remarks: 1/ In AddShare: you remove the 10 oldest entries and add 1? What I missed? 2/ AddShare: return !$dbh->err; => useless, $dbh->do insert returns the number of inserted rows. 3/ C4::VirtualShelves::ShelfPossibleAction Why do you use numeric code? It should be better to use an readable code. Moreover, these codes are used in the template. 4/ return if !defined($i) || !$i || $i eq '0E0'; #not found return 1; Could be replaced with return 1 if $i > 0 I think. 5/ It could be great to replace quotes with simple quotes around sql queries. 6/ (In reply to M. de Rooy from comment #31) > (In reply to Jonathan Druart from comment #26) > > - The new letter should be added to others languages. > I would really get rid of that restriction. I started some development on > that (look for i18n). But unfortunately there is not that much support for > it. Yes, but this one cannot be pushed as it and this one is not marked as dependent on the other bug report. 7/ opac/opac-shareshelf.pl + for( 0..$#$alphabet ) It could be good to name the variable loop. (In reply to Jonathan Druart from comment #70) > QA comment: Thanks for taking a look. > 1/ In AddShare: you remove the 10 oldest entries and add 1? What I missed? Just housekeeping. 10 is just an arbitrary number. (Could add this at some point in cleanup_database.) > 2/ AddShare: > return !$dbh->err; > => useless, $dbh->do insert returns the number of inserted rows. Not sure. Could that be 0E0 ? Think that this is safer. > 3/ C4::VirtualShelves::ShelfPossibleAction > Why do you use numeric code? It should be better to use an readable code. > Moreover, these codes are used in the template. Have been using error numbers in shareshelf everywhere. Would recommend to leave it as-is for now. > 4/ return if !defined($i) || !$i || $i eq '0E0'; #not found > return 1; > Could be replaced with > return 1 if $i > 0 > I think. Not sure about that too. > 6/ (In reply to M. de Rooy from comment #31) > > (In reply to Jonathan Druart from comment #26) > > > - The new letter should be added to others languages. > > I would really get rid of that restriction. I started some development on > > that (look for i18n). But unfortunately there is not that much support for > > it. > Yes, but this one cannot be pushed as it and this one is not marked as > dependent on the other bug report. I abandoned the i18n reports. Getting that into Koha is too much for me on my own. Even reports of this size are hard to get in. In the commit message I wrote that I will add the other 10? languages as soon as this is ready for a push. (It will only trigger more merge conflicts.) > 7/ > opac/opac-shareshelf.pl > + for( 0..$#$alphabet ) > > It could be good to name the variable loop. No big deal. Will do that. Created attachment 26458 [details] [review] Bug 9032: QA Follow-up: Use loop variable instead of $_ in opac-shareshelf (In reply to Jonathan Druart from comment #70) > 5/ It could be great to replace quotes with simple quotes around sql queries. Can we do that for C4/VirtualShelves.pm on another report (since there are much more double quotes over there) and not put that load here? Will open one for that purpose. Like to wait with code changes of that nature until this code moves.. (In reply to Jonathan Druart from comment #26) > QA comment: > - The new letter should be added to others languages. So what do you suggest for that? When will your provide a patch? After the push? > The ergonomic: > - The page opac-shelves.pl?display=privateshelves displays a link for > editing and deleting, but a button for sharing. Is it intentional? > - There is no back button if I enter a bad email address. It seems you did not answer on that. (In reply to M. de Rooy from comment #71) > (In reply to Jonathan Druart from comment #70) > > 1/ In AddShare: you remove the 10 oldest entries and add 1? What I missed? > Just housekeeping. 10 is just an arbitrary number. (Could add this at some > point in cleanup_database.) Yes, I am under the impression that the deletion should be done somewhere else. (In reply to Jonathan Druart from comment #74) Thanks again. > (In reply to Jonathan Druart from comment #26) > > QA comment: > > - The new letter should be added to others languages. > > So what do you suggest for that? When will your provide a patch? After the > push? As soon as the RM is willing to push, I will do it right away. > > The ergonomic: > > - The page opac-shelves.pl?display=privateshelves displays a link for > > editing and deleting, but a button for sharing. Is it intentional? > > - There is no back button if I enter a bad email address. > > It seems you did not answer on that. Sorry, could have missed that. I fixed the button for bootstrap btw. I left it in prog, since that will go away soon. If you want, I can send a followup for a back button. > (In reply to M. de Rooy from comment #71) > > (In reply to Jonathan Druart from comment #70) > > > 1/ In AddShare: you remove the 10 oldest entries and add 1? What I missed? > > Just housekeeping. 10 is just an arbitrary number. (Could add this at some > > point in cleanup_database.) > > Yes, I am under the impression that the deletion should be done somewhere > else. I suggest to leave it here now, but I will open a new report and move it then to cleanup-database.pl. Agreed? I think there are not enough changes to the sample notices to expect a lot of merge problems in that area. I'd prefer to have the patch ready when Galen reviews this, so there are no additional dependencies that can get lost or forgotten. Also the patch should be tested. Created attachment 26509 [details] [review] Bug 9032: QA Follow-up: Loop variable and Cancel button To satisfy QA needs: Use loop variable instead of $_ in opac-shareshelf.pl Adds Cancel button to Invite form (prog and bootstrap) in opac-shareshelf.tt. Likewise adds Return link under an error message in opac-shareshelf.tt. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Katrin Fischer from comment #76) > I think there are not enough changes to the sample notices to expect a lot > of merge problems in that area. I'd prefer to have the patch ready when > Galen reviews this, so there are no additional dependencies that can get > lost or forgotten. Also the patch should be tested. OK Katrin. I'll still try to do it.. We should have changed this.. :) Created attachment 26513 [details] [review] Bug 9032: QA Follow-up: Adding notices to foreign installer files Adds the notices SHARE_INVITE and SHARE_ACCEPT to sample_notices.sql for the foreign language installer files in installer/data/mysql. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I tested each installer file by deleting all notices from letter, running the file and counting the records. I did so with and without this patch. The results below show that in each case two notices were added as expected. Note that the English file contains 25 notices including the two new ones. BEFORE THIS PATCH: de-DE/mandatory/sample_notices.sql: 23 notices es-ES/mandatory/sample_notices.sql: 18 notices fr-FR/1-Obligatoire/sample_notices.sql: 18 notices it-IT/necessari/notices.sql: 21 notices nb-NO/1-Obligatorisk/sample_notices.sql: 23 notices pl-PL/mandatory/sample_notices.sql: 18 notices ru-RU/mandatory/sample_notices.sql: 18 notices uk-UA/mandatory/sample_notices.sql: 17 notices AFTER THIS PATCH: de-DE/mandatory/sample_notices.sql: 25 notices es-ES/mandatory/sample_notices.sql: 20 notices fr-FR/1-Obligatoire/sample_notices.sql: 20 notices it-IT/necessari/notices.sql: 23 notices nb-NO/1-Obligatorisk/sample_notices.sql: 25 notices pl-PL/mandatory/sample_notices.sql: 20 notices ru-RU/mandatory/sample_notices.sql: 20 notices uk-UA/mandatory/sample_notices.sql: 19 notices Created attachment 26577 [details] [review] Bug 9032: Share a list (part 1: send the invitation) Adds a Share button for OPAC private lists. Allows you to send an invitation to share a list. Checks on validity of email addresses (with Email::Valid). Test plan: 1) Sharing depends on syspref and login. Toggle the pref OpacAllowSharingPrivateList. If enabled, you should see the Share button in OPAC/Private lists. Click on the Share button. You should get Share a list. Logout and try to go back to opac/opac-shareshelf.pl It should now present you the login form. 2) Try to share a public list or a list you do not own. Find a security hole in the interface. Or hack the shareshelf URL and replace the shelfnumber with a public list number. 3) Enter no email address or invalid ones (no domain, forbidden chars). If you enter no address, submit should not work. If you enter only wrong addresses (separated by: ,:; ), you get a message. 4) Test if sending the invitation works. Share one of your private lists. Enter your own email address. After your proc_message_queue cronjob ran, you should have an email. Check also if you see a new record in the virtualshelfshares table. Note that the followup patch handles the second part of accepting this share. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased. Small amendation: rewording on email notification in template. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26578 [details] [review] Bug 9032: Followup for new dependency Email::Valid Just adds this dependency to PerlDependencies module. Note from Robin: libemail-valid-perl is in squeeze already. Test plan: You should see Email::Valid now under About/Perl modules. If Email::Valid is not yet installed on your system, please do so. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26579 [details] [review] Bug 9032 [Template follow-up] Share a list (part 1: send the invitation) This patch makes a few changes to the "share a list" template: - We don't use tables for layout of forms. This patch corrects the display of the share form to use Koha standard form markup. - Revise the confirmation message displayed after a list is shared to be more readable. - Add a link back to private lists to the confirmation message. - Add T:T markup to make this page display the left-hand navigation which is shown on other patron-related pages when a user is logged in. To test, apply this patch on top of other patches pending for Bug 9032 and confirm that the list share form looks correct and works correctly. Confirm that share confirmation page reads well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased March 5, 2014. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26580 [details] [review] Bug 9032: [QA Follow-up] Plackify opac-shareshelf.pl Plackified opac-shareshelf by moving global my variables into hashref. Slightly changed base64ord and base64chr functions to more generic ones. Test plan: Verify if sending an share invitation still works (under prog theme). Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26581 [details] [review] Bug 9032: Share a list (accept invitation and remove a share) This patch handles: [1] The response (acceptance) by the invited person. If he accepts this share, the private list of the sender will be shown under Your lists on the shelves page. In OPAC 'Your private lists' has been renamed to Your lists (just as in Staff). The Type column shows Private or Shared for these lists; a list appears as Shared as soon as an invitation has been accepted. The owner has the options to Edit, Delete or Share; the invited person does not have these options on the shared list. [2] Removing an accepted share. If a user accepted a share, he should also be able to remove it again. The Remove Share button is visible on OPAC when viewing Your lists or a particular shared list. Note: AddShare has been extended to return a possible database error. If the share invite could not be added, a mail will not be sent. Test plan (for prog theme): Enable pref OpacAllowSharingPrivateLists User 1 creates new private list P1, perms: D-A-D, adds 2 items, sends share User 1 checks your lists display: is P1 Private with Edit button? User 2 accepts share: sees P1, but cannot add or delete items User 2 checks your lists display again: P1 shows Shared without Edit? User 1 checks your lists display again: P1 shows Shared with Edit? User 2 tries to accept share again: should fail now User 3 tries to accept share: should also fail User 3 tries again, modifies shelfnumber and/or key in url: should also fail User 2 creates new private list P2, perms: A-A-A, no items, sends share User 2 checks your lists display: P2 shows Private with Edit? User 1 accepts, adds one item User 1 checks your lists display: P2 shows Shared without Edit? User 2 checks your lists display: P2 shows Shared with Edit? User 2 deletes item of user 1 (allowed) User 2 deletes list P2 User 1 checks your lists display in opac or staff: P2 is gone? User 1 creates private list P3, sends a share. User 1 creates private list P4, adds one item, sends a share. User 2 accepts the share for P3. User 2 checks the shelves display, and removes share P3. User 2 accepts the share for P4. User 2 views shelf P4 with one item and confirms Remove share on that form. User 2 checks shelves display again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Combined changes for AcceptShare and RemoveShare. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26582 [details] [review] Bug 9032: Unit tests for sharing lists This patch combines some work from report 10389 and 10441. With thanks to Srdjan and Jonathan Druart for their contributions. Adds tests for AddShare, AcceptShare, IsSharedList, RemoveShare. This patch also restores the inadvertently deleted DelShelf tests in bug 10499. Test plan: Run the test VirtualShelves.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased and squashed on March 4, 2013. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26583 [details] [review] Bug 9032: Consistent behavior when deleting lists DelShelf deletes a list regardless whether it is private, shared or public. HandleDelBorrower had another approach, trying to save shared and public lists by setting the owner to NULL. This patch makes both routines behave consistent. A new report 11889 has been opened to discuss the 'disowning' feature. NOTE: I did not add a db revision here to handle possible cases of lists without owner in the current data. Such public (or shared) lists can still be used without any problem. Bug 11889 and a new planned report for a lists management tool will address this topic further on. After that, all goals of umbrella report 7310 should be realized. Test plan: Create a list P1 with user1 that allows adding by other users. Add a patron (user2). Login as user2 and create some lists, add some items. Let user2 add some entries to P1 too. Delete patron user2. Verify that his lists are gone, but his entries in P1 are kept (nullified). Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26584 [details] [review] Bug 9032: Follow-up for bootstrap theme Adds new template opac-shareshelf.tt. Modifies template opac-shelves.tt: Share button, Remove Share button, label "Your lists" instead of "Your private lists", list category Shared. Test plan: Verify if the Share a list-features work in bootstrap by: [1] Switch to bootstrap. Go to Lists. [2] Share one of your private lists. [3] Login as another user and accept the invitation. [4] Remove the share again. [5] Check if Share and Remove share do not popup for public lists. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26585 [details] [review] Bug 9032: Share a list (database revision for notices) Adds notice SHARE_INVITE in updatedatabase and sample_notices. [Amendation: also adds SHARE_ACCEPT notice.] Test plan: Install or upgrade and verify that you have the new notice under Tools. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased on March 3, 2014. Amended the patch to add the second notice SHARE_ACCEPT. Tested upgrade with updatestructure step of web installer. Tested install by deleting all notices and running sample_notices.sql NOTE: Will add the notices to the other sql files in the other language directories as soon as this set of patches is ready to be pushed in order to reduce the need to rebase them constantly. Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26586 [details] [review] Bug 9032: QA Follow-up: Send cookie with redirect Small fix to resolve a problem Dobrica noted on the report. If you had to login when accepting the share, the cookie was not passed back to the server when redirecting to shelf display. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Accepted a share while logged in and while not logged in. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26587 [details] [review] Bug 9032: QA Follow-up: Loop variable and Cancel button To satisfy QA needs: Use loop variable instead of $_ in opac-shareshelf.pl Adds Cancel button to Invite form (prog and bootstrap) in opac-shareshelf.tt. Likewise adds Return link under an error message in opac-shareshelf.tt. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26588 [details] [review] Bug 9032: QA Follow-up: Adding notices to foreign installer files Adds the notices SHARE_INVITE and SHARE_ACCEPT to sample_notices.sql for the foreign language installer files in installer/data/mysql. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I tested each installer file by deleting all notices from letter, running the file and counting the records. I did so with and without this patch. The results below show that in each case two notices were added as expected. Note that the English file contains 25 notices including the two new ones. BEFORE THIS PATCH: de-DE/mandatory/sample_notices.sql: 23 notices es-ES/mandatory/sample_notices.sql: 18 notices fr-FR/1-Obligatoire/sample_notices.sql: 18 notices it-IT/necessari/notices.sql: 21 notices nb-NO/1-Obligatorisk/sample_notices.sql: 23 notices pl-PL/mandatory/sample_notices.sql: 18 notices ru-RU/mandatory/sample_notices.sql: 18 notices uk-UA/mandatory/sample_notices.sql: 17 notices AFTER THIS PATCH: de-DE/mandatory/sample_notices.sql: 25 notices es-ES/mandatory/sample_notices.sql: 20 notices fr-FR/1-Obligatoire/sample_notices.sql: 20 notices it-IT/necessari/notices.sql: 23 notices nb-NO/1-Obligatorisk/sample_notices.sql: 25 notices pl-PL/mandatory/sample_notices.sql: 20 notices ru-RU/mandatory/sample_notices.sql: 20 notices uk-UA/mandatory/sample_notices.sql: 19 notices Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Thanks for QA, Jonathan. Pushed to master, along with my usual set of follow-ups. One follow-up deserving special mention is the last one, which restores the documented interpretation of the sharedate column. Thanks, Marcel! (In reply to Galen Charlton from comment #93) > Pushed to master, along with my usual set of follow-ups. One follow-up > deserving special mention is the last one, which restores the documented > interpretation of the sharedate column. Thanks for the follow-ups. Retested the feature successfully in current master along with new parameter of cleanup-database. |