https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37893
Pedro Amorim <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA --- Comment #61 from Pedro Amorim <[email protected]> --- Thank you Jonathan for the time spent here, as always. I've done my best to address your feedback below. (In reply to Jonathan Druart from comment #57) > There are tons of failures caught by the QA script. Yes, please read comment 42: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37893#c42 I believe most of these are false positives, however the following follow-up patches fix those that aren't: 1) Set tinyint boolean => 0 for tinyint not boolean fields: https://github.com/openfifth/koha/commit/521a53224c4d364e2910397a801a1c4af252b9c5 2) Add new line to apache-shared-intranet.conf: https://github.com/openfifth/koha/commit/ea30ba55dd978c92e33e6f270a8e7931e7d79d69 3) js-patron-format.inc (only shows now after bug 40958): https://github.com/openfifth/koha/commit/4ee64da97a6e7b7d60cec65131fd3ea925da3365 (In reply to Jonathan Druart from comment #58) > Quick QA review (not tested SIP) > > 1. There are no cypress tests Yes, there are. See t/cypress/integration/SIP2. > 2. Only use the plural module is needed > eg. > 22 use Koha::SIP2::Account; > 23 use Koha::SIP2::Accounts; > same pattern in several places Patch addressing: https://github.com/openfifth/koha/commit/c7b1ee47073b6e1e94f92f8cc6a8fa3e4f1ba6b2 > 3. Tests are missing for (at least) Koha/SIP2/Account.pm > Also get_for_config sub are not covered by tests. Not sure how it would be > useful however... Tests added for Koha/SIP2/Account.pm: https://github.com/openfifth/koha/commit/3a07ee8194eb95a2897afc2c3e44a796c7b70975 get_for_config for all SIP related classes is tested by SIP2ModuleMigration.t. This test file tests and proves that the configuration read by the SIPServer is the same before and after the migration from XML to the database, which invokes get_for_config for each of the classes. > 4. If I upgrade I do have stuffs in the DB (picked from > /etc/koha/sites/kohadev/SIPconfig.xml) but for new installs the table are > not populated. > Maybe we need to patch miscdev(?) Out of scope here? AFAIK the 'dummy' existing config in SIPconfig.xml served only as documentation for all the possible configuration values for accounts + institutions + syspref overrides (which is what is migrated here). Server params and listeners are still read from the SIPconfig.xml file (as per DCook's original QA requirements). > 5. "Cash register:" shouldn't we hide it if none exists or if the pref is > off? Patch submitted to bug 41214. I agree with this, however I think a wider 'add option for resource attributes to display based on sys pref' to the VueJS framework is needed here. For this reason, I think this makes more sense as a follow-up bug, to be addressed and discussed in bug 41214. I don't think this should be a blocker here as: 1) The field is optional 2) Information regarding UseCashRegisters sys pref being required is also included in the field's respective tooltip. > 6. Framework bugs? > a. you can enter a letter after a number ("42a") in the numeric input (eg. > "retries") and submit the form and you get a "400 Something went wrong: > Error: Expected integer - got string." > b. you can enter a letter in the number inputs ('x') and it becomes "NaN" Yes, opened bug 41215, with reproduction instructions using existing vendors already in main (i.e. unrelated to the patchset here). > 7 Don't we want a config flag to turn off the ability to edit from the UI? I don't think so? The ability to access and work with the module is already governed by the new 'sip2' permission. > 8. kohastructure.sql has wrong DROP/CREATE statement > > +DROP TABLE IF EXISTS `file_transports`; > +/*!40101 SET @saved_cs_client = @@character_set_client */; > +/*!40101 SET character_set_client = utf8 */; > +CREATE TABLE `file_transports` ( Thank you for spotting this. It must've been caused by a failed past rebase. Fixed here: https://github.com/openfifth/koha/commit/26438204c16d1deaea2757cdd165de0aacbbdb38 > 9. I think sip2/sip2.tt is wrong about #sip2 being closed in > intranet-bottom.inc > It should be <div id="sip2"></div> and the last closing div removed. Patch addressing: https://github.com/openfifth/koha/commit/853f1365103c5ef747d7e136828ce75c21e2bebb > 10. Breadcrumb start with SIP2, shouldn't it be "Administration"? Patch addressing: https://github.com/openfifth/koha/commit/a704e629f5d96b16a284d1e4526de95dcd4c9ce7 (In reply to David Cook from comment #60) > Nice catches there, Jonathan. > > (In reply to Jonathan Druart from comment #59) > > 11. Missing changes in etc/koha-httpd.conf? > > I haven't checked all the places Jonathan mentioned but I have checked this > one and in light of bug 41167, I'm marking this as Failed QA Thank you for bringing my attention to bug 41167, I was not aware of it. However bug 41167 is not in main. I don't fully agree with FQA due to a bug not yet in main, I think it opens a less then ideal precedent, though I understand that it makes sense in this instance. Here is what I did: 1) Fully rebased the branch on current main 2) Made it depend on bug 41167 3) Added the patch: https://github.com/openfifth/koha/commit/57c5b64943ab72a744c2f9fcadf4c8b21e9d0b31 -- 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/
