|
Description
Jonathan Druart
2017-01-24 15:19:44 UTC
Created attachment 59508 [details] [review] Bug 17981: Move svc/letters to svc/letters/get To allow another script under svc/letters Created attachment 59509 [details] [review] Bug 17981: Add a 'letter' param to GetPreparedLetter This will allow us to call GetPreparedLetter with a letter content that does not exist in the DB Created attachment 59510 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). These patches will not apply due to the big dependency tree. Use this remote branch instead: https://github.com/joubu/Koha/tree/bug_17981 Created attachment 59514 [details] [review] Bug 17981: Better error handling Created attachment 59524 [details] [review] Bug 17981: Move the divs to the top of the modal I have rebased this branch, that was a nightmare, really... There are too many patches in the queue, please help me to move things toward master. Created attachment 63978 [details] [review] Bug 17981: Move svc/letters to svc/letters/get To allow another script under svc/letters Created attachment 63979 [details] [review] Bug 17981: Add a 'letter' param to GetPreparedLetter This will allow us to call GetPreparedLetter with a letter content that does not exist in the DB Created attachment 63980 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Created attachment 63981 [details] [review] Bug 17981: Better error handling Created attachment 63982 [details] [review] Bug 17981: Move the divs to the top of the modal Is this patch set in a testable state yet? I applied the patches to master and did not see the preview button. Can you add a test plan? (In reply to Kyle M Hall from comment #13) > Is this patch set in a testable state yet? I applied the patches to master > and did not see the preview button. Can you add a test plan? You need to apply all patches under bug 17961, then test for CHECKIN, CHECKOUT or HOLD_SLIP. Then the commit message should contain all the information to test the patch. I believe the issue Kyle speaks of is an issue related to the cache. When I first applied it to master, I did not see the Preview button either on any of the three notices. However, I cleared the cache and then they appeared. I'd like to do additional testing before signing off. Also, this is one of my favorite enhancements. Our clerk who handles all the notices will be very grateful for this feature! I tested with the HOLD_SLIP notice on BibLibre's sandbox03. When I applied bug 17975, a new box appeared to preview the notice for "biblionumber|borrowernumber". I tested with record #3194 and 2 different patrons (#5903 with the hold waiting for her and #6587 who is the second in priority list) but for none of them I succeeded to preview the notice, it doesn't stop loading. Maybe I misunderstood something in the way to use it. I couldn't test it for CHECKIN and CHECKout, patch 17964 didn't apply. I do not think you can test this patch and all the dependencies on a sandbox. Created attachment 65688 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Created attachment 65689 [details] [review] Bug 17981: Better error handling Created attachment 65690 [details] [review] Bug 17981: Move the divs to the top of the modal > The possibilities: > - Mock data to get fully working generated messages for any notice > templates. For instance, for CHECKIN and CHECKOUT, the item is not > checked in/out yet. So we cannot access the issue's information. At first glance, I'm not seeing any big problems with this. I suppose we could have the librarian pass in an old_issues issue_id, but that seems like it would be a fair bit less user friendly. > - Browse the data to get the ones we want to use for the preview (big). That's a really great idea, but it does feel like it will be quite a lot of work, and typically librarians will probably just set up dummy patrons / titles for testing, so I doubt there is an immediate need for that. I also agree the current way of filling in the preview data is not user friendly ; ) Hopefully it won't be too much harder to put those data needed in separate form inputs. Created attachment 65701 [details] [review] Bug 17981: Move svc/letters to svc/letters/get To allow another script under svc/letters Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 65702 [details] [review] Bug 17981: Add a 'letter' param to GetPreparedLetter This will allow us to call GetPreparedLetter with a letter content that does not exist in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 65703 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 65704 [details] [review] Bug 17981: Better error handling Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 65705 [details] [review] Bug 17981: Move the divs to the top of the modal Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 67144 [details] [review] Bug 17981: Move svc/letters to svc/letters/get To allow another script under svc/letters Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 67145 [details] [review] Bug 17981: Add a 'letter' param to GetPreparedLetter This will allow us to call GetPreparedLetter with a letter content that does not exist in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 67146 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 67147 [details] [review] Bug 17981: Better error handling Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 67148 [details] [review] Bug 17981: Move the divs to the top of the modal Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> While rebasing Bug 18007 I noticed something in this update: In the JavaScript there are couple of instances of a "module" variable which doesn't appear to ever be initialized. Is it superfluous, or should it be created somewhere? (In reply to Owen Leonard from comment #34) > While rebasing Bug 18007 I noticed something in this update: > > In the JavaScript there are couple of instances of a "module" variable which > doesn't appear to ever be initialized. Is it superfluous, or should it be > created somewhere? Yes you are right, it could be removed for now. Created attachment 67152 [details] [review] Bug 17981: Remove uneeded module JS var As we only support CHECKIN and CHECKOUT for now, we do not need this module variable. It may be useful to re-add it later. Created attachment 70300 [details] [review] Bug 17981: Move svc/letters to svc/letters/get To allow another script under svc/letters Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 70301 [details] [review] Bug 17981: Add a 'letter' param to GetPreparedLetter This will allow us to call GetPreparedLetter with a letter content that does not exist in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 70302 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 70303 [details] [review] Bug 17981: Better error handling Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 70304 [details] [review] Bug 17981: Move the divs to the top of the modal Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 70305 [details] [review] Bug 17981: Remove uneeded module JS var As we only support CHECKIN and CHECKOUT for now, we do not need this module variable. It may be useful to re-add it later. Please rebase! Created attachment 73201 [details] [review] Bug 17981: Move svc/letters to svc/letters/get To allow another script under svc/letters Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73202 [details] [review] Bug 17981: Add a 'letter' param to GetPreparedLetter This will allow us to call GetPreparedLetter with a letter content that does not exist in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73203 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73204 [details] [review] Bug 17981: Better error handling Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73205 [details] [review] Bug 17981: Move the divs to the top of the modal Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73206 [details] [review] Bug 17981: Remove uneeded module JS var As we only support CHECKIN and CHECKOUT for now, we do not need this module variable. It may be useful to re-add it later. Patches do not apply on master. Please rebase.
> Applying: Bug 17981: Move svc/letters to svc/letters/get
> Applying: Bug 17981: Add a 'letter' param to GetPreparedLetter
> Applying: Bug 17981: Add a preview mode for notice templates
> error: sha1 information is lacking or useless (C4/Letters.pm).
> error: could not build fake ancestor
> Patch failed at 0001 Bug 17981: Add a preview mode for notice templates
Created attachment 73949 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73950 [details] [review] Bug 17981: Better error handling Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73951 [details] [review] Bug 17981: Move the divs to the top of the modal Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 73952 [details] [review] Bug 17981: Remove uneeded module JS var As we only support CHECKIN and CHECKOUT for now, we do not need this module variable. It may be useful to re-add it later. Looks good, just one thing so far:
When the notice is in TT syntax, the svc/letters/preview and with ISE: Can't use string ("") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/svc/letters/preview line 238.
Created attachment 74025 [details] [review] Bug 17981: Do not crash if the template is already using TT syntax Created attachment 74027 [details] [review] Bug 17981: Move svc/letters to svc/letters/get To allow another script under svc/letters Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74028 [details] [review] Bug 17981: Add a 'letter' param to GetPreparedLetter This will allow us to call GetPreparedLetter with a letter content that does not exist in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74029 [details] [review] Bug 17981: Add a preview mode for notice templates This patch is a first step to provide a preview mode for notice templates. CHECKIN, CHECKOUT and HOLD_SLIP are supported so far. Maybe more, but I have not tested yet and the interface will not allow you to generate the preview. The idea is to provide an idea of how will render the messages generated from a notice template. A new "Preview" button is added close to each textarea on the editing notice templates view. For each notice template code (letter_code), we will need some input data to produce the preview. For instance, for CHECKIN we need an barcode. From the barcode we can guess all the other data. For CHECKOUT we will need the borrowernumber and the barcode. Note that the way to enter the data for the preview is not really user-friendly, for CHECKOUT you will have to fill 'borrowernumber|barcode', but the placeholder will help you to know how and what to fill. In the modal window, you will see 4 blocks: 1/ the content of the letter (with the placeholds << >>) 2/ the generated message (with the data filled) 3/ if the letter contained historical syntax markers, the screen will try to generate a notice template using the TT syntax 4/ the generated message from this TT syntax => You will be able to compare the 2 generated messages. What is the goal of this first patchset: - Show this first POC and get feedback from other developpers - Add a way to easily visualise the differences between the 2 syntaxes - Confort users with the TT syntax and the migration step from the historical syntax. I'd like to get opinions before going further. The possibilities: - Mock data to get fully working generated messages for any notice templates. For instance, for CHECKIN and CHECKOUT, the item is not checked in/out yet. So we cannot access the issue's information. (I have no idea how to do that) - Browse the data to get the ones we want to use for the preview (big). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74030 [details] [review] Bug 17981: Better error handling Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74031 [details] [review] Bug 17981: Move the divs to the top of the modal Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74032 [details] [review] Bug 17981: Remove uneeded module JS var As we only support CHECKIN and CHECKOUT for now, we do not need this module variable. It may be useful to re-add it later. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74033 [details] [review] Bug 17981: Do not crash if the template is already using TT syntax Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74034 [details] [review] Bug 17981: (QA follow-up) Fix and update pod in svc/letters/get Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Pushed to master for 18.05, thanks to everybody involved! Enhancement, not backported for 17.11.x Going to double-check but this might be broken in 19.11... (In reply to David Cook from comment #67) > Going to double-check but this might be broken in 19.11... I must've been mistaken as it looks fine now. |