Description
Tomás Cohen Arazi (tcohen)
2011-05-04 15:25:13 UTC
Created attachment 4398 [details] [review] Initial patch can you include 'how to test' instructions here? To test: Make a normal install, choose 'MARC Code List for Relators, as of http://www.loc.gov/marc/relators/relaterm.html (marc21_relatorterms)" in the Optional data section of the Choose frameworks step. Then edit a MARC Framework and choose to link 700$e to the list of authorized values MARCRELATOR Success should be: - You can choose to install MaRC relator terms data - You can link to that list in your frameworks Created attachment 4577 [details] [review] Fix category name lenght MARCRELATOR is larger than the 10 char limit of the field. It is now called RELTERMS. Okay, and how do we test this if we're a library upgrading - not a new install? There should be an option for those who are on Koha already and just upgrading from an older version (as I am on my test machine). Nicole Prompting the user for loading 'new in koha' sample data is not a feature Koha provides right now. Maybe there could be a new bug filled for that. As of this bug, it should be signed-off if it happens to work on new installs, as any other optional data's SQL files. Not tested on a new install, but with $ mysql -u user -p db_name < installer/data/mysql/en/optional/marc21_relatorterms.sql and it works fine. Waiting for someone testing on a new install. > There should be an option for those who are on Koha already and just upgrading
from an older version (as I am on my test machine).
I think this patch provides an excellent addition of sample MARC data. The install script offers the option as advertised. (Did not run it though.)
But I agree with Nicole that only this patch will not help existing installs. An experienced user may run mysql from the command-line, but that seems not to be enough. (How would he know at first?) An informational warning on the updatedatabase screen could be helpful as well as additional information in the INSTALL files. (But even these two things may be overlooked easily.)
Additionally, minor point: Should you better test on the existence of RELTERMS in the table before inserting? I already have RELTERM in my data; could easily have been RELTERMS too. If RELTERMS should exist somewhere already, you would probably want to insert them under a different name.
Marcel, (In reply to comment #8) > > There should be an option for those who are on Koha already and just upgrading > from an older version (as I am on my test machine). As stated before, I asked on this on IRC and I was told there is no way to handle this properly (i.e. prompt the user to make desitions on this). As I see it, this is not different to sample patron types and such. If you think I should add an advice on updatedatabase.pl just let me know. If you think we should have a method for prompting the user to make a desition just let me know, and we can fill a new bug to work on that. Regards To+ (In reply to comment #9) > As I see it, this is not different to sample patron types and such. I think you are right about that. I would however add at least a statement to the upgrading user via the updatedatabase script telling him that he could add relator terms now (if you mention that you are adding under RELTERMS, you partially cover my other remark about the name too). So, I would be willing to sign off. I now tested the patch on an empty db. That worked okay. But unfortunately I saw then that several of the descriptions are somehow truncated in your list (sound should be sound designer, supporting should be supporting host, etc.) It seems that a lot of the two-or-more-words descriptions only contain one word. (You have 80 chars there.) Therefore I suggest to let somebody correct this list first and add a line to updatedatabase. Created attachment 4847 [details] [review] Complete list, complete strings As noted by Marcel, strings where truncated. Fixed that. Created attachment 4848 [details] [review] advertise availability in updatedatabase.pl I put it in a different patch so the resubmited patch when need to rebase is smaller :-P I assume 3 or 4 rebases follow. Created attachment 4855 [details]
Screenshot of updatedb page
Printing a '\n' in updatedatabase.pl cause lines to be treated as html list items (see screenshot). I think you must keep only the last line feed (just esthetical issue however).
Created attachment 4890 [details] [review] advertise availability in updatedatabase.pl (single line) If anyone is about to sign this please wait until I provide a es-ES script, now that language is on master. (In reply to comment #15) > If anyone is about to sign this please wait until I provide a es-ES script, now > that language is on master. Tomas, I am signing off now on both patches. Please send a small followup fix for Spanish. Created attachment 4937 [details] [review] Signed patch (complete strings) Created attachment 4938 [details] [review] Signed patch (updatedatabase advertisement) Created attachment 4940 [details] [review] es-ES was missing es-ES translation will come when our happy librarians finish translating. Created attachment 4957 [details] [review] Signed follow up patch for Spanish Created attachment 5735 [details] [review] Bug 6299 - add es-ES Translation will come next, when our librarians finish the translation. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 11, 2011: Follow up for Spanish signed off. Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 5736 [details] [review] Bug 6299 - Advertise this addition during upgrade Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 10, 2011: Fixed some small typos. Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Oct 7th, 2011: rebased for master QA comment: * installer only patch * add some datas in authorised values, nothing "dangerous" My only concern is that fr, de, ... versions are also in english. But that can be addessed in another patch, and should not delay passed QA imo so, marking "passed QA", but chris, I wouldn't complain if you set "failed QA" because of the language comment I made updatedatabase.pl patch has <<<<<<<<< HEAD relic. I recommend that we remove any of the SQL files for languages that do not have their translations in place. It's extra work for a system administrator to remove those untranslated values after they're added, and the lack of support for certain languages is not advertised. The sysadmin will still have the choice to upload an existing language. In addition to removing these untranslated SQL files, I also recommend altering the updatedatabase.pl script to indicate exactly which languages are currently supported. Marking as Failed QA until the git merge relic is removed, and the recommendations for handling untranslated files is addressed. yikes ! sorry for the <<<< relic, it's me !!! OK for the translation proposal, that's a good option. Created attachment 7371 [details] [review] Bug 6299 - Provide a list of authorized values for Relator terms Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 10,2011: Tested install and upgrade. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> January 28, 2012: Removed untranslated non-en SQL relator term files. Created attachment 7372 [details] [review] Bug 6299 - Advertise this addition during upgrade Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 10, 2011: Fixed some small typos. Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Oct 7th, 2011: rebased for master Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> January 12, 2012: Noted that RELTERMs are currently available only for en. Created attachment 7373 [details] [review] Bug 6299 - Provide a list of authorized values for Relator terms Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 10,2011: Tested install and upgrade. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> January 28, 2012: Removed untranslated non-en SQL relator term files. Created attachment 7374 [details] [review] Bug 6299 - Advertise this addition during upgrade Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 10, 2011: Fixed some small typos. Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Oct 7th, 2011: rebased for master Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> January 12, 2012: Noted that RELTERMs are currently available only for en. Made the changes requested by Ian, and returning the status to signed off. Note that the "add es-ES" patch should *not* be applied, as it has not been translated yet. tcohen's comment indicates that a translation is in the works. Created attachment 7378 [details] [review] Bug 6299 - Advertise this addition during upgrade Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 10, 2011: Fixed some small typos. Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Oct 7th, 2011: rebased for master Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> January 12, 2012: Noted that RELTERMs are currently available only for en. Oops. I didn't actually change the status. The MARC Code List for Relators is here: http://www.loc.gov/marc/relators/relacode.html The translation of this table in French is available here: http://www.marc21.ca/040010-220-f.html Created attachment 7389 [details] [review] Relator codes in French Additional sign-off required for 'Relator codes in Frenche' patch. Created attachment 7408 [details] [review] Relator codes in French v2: fixing typos To test: Add it to your db cd <koha-root>/installer/data/mysql/fr-FR/marcflavour/marc21/Optionnel cat marc21_relatorterms.sql | mysql <koha-db> -u <koha-user> -p <koha-pwd> In Koha pro > Administration > Authorized values, you have a new RELTERMS list containing added relator codes. I've been out on vacations. Next week I'll provide a new patch for the spanish translation which I have at the office. Created attachment 7490 [details] [review] Relator codes in French Fix a typo thanks to Tomás. Created attachment 7500 [details] [review] Bug 6299 Add relator codes for fr-FR Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Confirmed that the RELTERMS authorised values category is populated. Not knowing French I can't comment on the translations, but any errors can be corrected at a later date. Created attachment 7501 [details] [review] Bug 6299 - add es-ES Spanish translation added, no signoff yet. Created attachment 7509 [details] [review] Bug 6299 - add es-ES Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Confirmed that the RELTERMS authorised values category is populated. Not knowing Spanish I can't comment on the translations, but any errors can be corrected at a later date. QA comment: Those patches look OK, however, when trying to apply the FR & ES translations, I get a nasty: fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 6299 - add es-ES When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". (same for FR) Any idea why Jared ? (In reply to comment #43) > QA comment: > > Those patches look OK, however, when trying to apply the FR & ES translations, > I get a nasty: > > fatal: sha1 information is lacking or useless > (installer/data/mysql/updatedatabase.pl). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 Bug 6299 - add es-ES > When you have resolved this problem run "git am --resolved". > If you would prefer to skip this patch, instead run "git am --skip". > To restore the original branch and stop patching run "git am --abort". > > (same for FR) > > Any idea why Jared ? There must have been an addition to updatedatabase.pl that makes patches don't apply Created attachment 7637 [details] [review] Bug 6299 Add relator codes for fr-FR Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Confirmed that the RELTERMS authorised values category is populated. Not knowing French I can't comment on the translations, but any errors can be corrected at a later date. Created attachment 7638 [details] [review] Bug 6299 - add es-ES Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Confirmed that the RELTERMS authorised values category is populated. Not knowing Spanish I can't comment on the translations, but any errors can be corrected at a later date. Created attachment 7639 [details] [review] Bug 6299 - Advertise this addition during upgrade Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> August 10, 2011: Fixed some small typos. Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Oct 7th, 2011: rebased for master Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased to fix the problem that Paul encountered. Reordered the patches to simplify later rebasings. Returning to Passed QA status. perfect, this time, worked well ! There have been no further reports of problems so I am marking this bug resolved. |