https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35026
Katrin Fischer <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #101 from Katrin Fischer <[email protected]> --- Patches still apply cleanly, QA script is green. So far so good! This is quite a big piece of code and as such hard to test. I think it might have been nice to move the code in smaller bits. But overall this is of course quite an improvement for the unit tests alone. Trusting Nick to have done a thorough review here considering him our current expert for this specific piece of code. Some questions remain: 1) Koha/MarcOrder.pm I was wondering if that is the right spot for the new module and if it should not be within Acquisition or Import? It feels a bit like it's missing context or being something in the middle. 2) Beware terminology! marc record based on the mappings in the syspref MARC record based on the mappings in the system preference... biblio records bibliographic records Some more marc vs. MARC: Couldn't translate marc information 3) _format_price_to_CurrencyFormat_syspref This one feels a bit risky. As it's now it assumes that the chosen display format also governs the format of data that a library might want to import. It doesn't check where the comma is in the string, but blindly replaces. I can see it's done multiple times in the original code, so we will keep the behavior consistent. I'd still question if it's in the right spot being in the new MarcOrder module and if it should not be somewhat refined. Maybe a case for a follow-up bug for later? -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
