Bug 14889

Summary: Move the framework related code to Koha::BiblioFramework[s]
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: bgkriegel, brendan, josef.moravec, katrin.fischer, kyle, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15449, 15801, 16807    
Attachments: Bug 14889: Add Koha::BiblioFramework[s] classes
Bug 14889: Use Koha::BiblioFramework[s] in admin/biblio_framework.pl
[SIGNED-OFF] Bug 14889: Add Koha::BiblioFramework[s] classes
[SIGNED-OFF] Bug 14889: Use Koha::BiblioFramework[s] in admin/biblio_framework.pl
Bug 14889: QA Follow-up
Bug 14889: Fix error msg on duplicate
Bug 14889: Add tests for Koha::BiblioFramework[s]
Bug 14889: Add Koha::BiblioFramework[s] classes
Bug 14889: Use Koha::BiblioFramework[s] in admin/biblio_framework.pl
Bug 14889: QA Follow-up
Bug 14889: Fix error msg on duplicate
Bug 14889: Add tests for Koha::BiblioFramework[s]
[PASSED QA] Bug 14889: Add Koha::BiblioFramework[s] classes
[PASSED QA] Bug 14889: Use Koha::BiblioFramework[s] in admin/biblio_framework.pl
[PASSED QA] Bug 14889: QA Follow-up
[PASSED QA] Bug 14889: Fix error msg on duplicate
[PASSED QA] Bug 14889: Add tests for Koha::BiblioFramework[s]
Bug 14889: Fix typo in Koha/BiblioFrameworks.pm

Description Jonathan Druart 2015-09-24 15:08:01 UTC

    
Comment 1 Jonathan Druart 2015-09-24 15:10:44 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-09-24 15:10:47 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2015-10-22 18:09:31 UTC Comment hidden (obsolete)
Comment 4 Bernardo Gonzalez Kriegel 2015-10-22 18:09:40 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2015-10-25 09:36:44 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2015-10-25 09:38:58 UTC
I have added a follow-up - but there are 2 things:

1) Please add some unit tests.
2) When you add a new framework with the same frameworkcode than an existing framework, I think it should display the appropriate message, but instead, it updates the existing framework description. On current master, you have a silent SQL erorr in the logs, but the existing framework is left unchanged. (not ideal either...)

Otherwise this works quite well, so almost there!
Comment 7 Jonathan Druart 2015-10-26 08:57:16 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-10-26 08:57:19 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-11-05 12:17:56 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-11-05 12:17:59 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-11-05 12:18:04 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-11-05 12:18:07 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-11-05 12:18:10 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2015-11-19 10:43:10 UTC
Created attachment 44985 [details] [review]
[PASSED QA] Bug 14889: Add Koha::BiblioFramework[s] classes

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2015-11-19 10:43:14 UTC
Created attachment 44986 [details] [review]
[PASSED QA] Bug 14889: Use Koha::BiblioFramework[s] in admin/biblio_framework.pl

Test plan:
Add/edit/remove biblio frameworks from the administration module
(admin/biblio_framework.pl).
You should get message feedback after each action.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No problems found. Tested add/edit/delete
No koha-qa errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2015-11-19 10:43:17 UTC
Created attachment 44987 [details] [review]
[PASSED QA] Bug 14889: QA Follow-up

- Removes unused package from BiblioFramework.pm
- Makes framework table sortable
- Some small changes in wording of error messages

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2015-11-19 10:43:20 UTC
Created attachment 44988 [details] [review]
[PASSED QA] Bug 14889: Fix error msg on duplicate

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2015-11-19 10:43:24 UTC
Created attachment 44989 [details] [review]
[PASSED QA] Bug 14889: Add tests for Koha::BiblioFramework[s]

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Brendan Gallagher 2015-12-31 19:10:00 UTC
Pushed to master - Thanks!
Comment 20 Jonathan Druart 2016-02-10 15:59:59 UTC
Created attachment 47857 [details] [review]
Bug 14889: Fix typo in Koha/BiblioFrameworks.pm
Comment 21 Jonathan Druart 2016-02-10 16:00:53 UTC
Just found a typo in the patchset, @RM could you please push the *last* patch please?
Comment 22 Katrin Fischer 2016-02-28 21:01:18 UTC
Has ths happened yet?
Comment 23 Katrin Fischer 2016-02-28 21:01:43 UTC
Ah, all need to be pushed still. Sorry, misunderstood!
Comment 24 Brendan Gallagher 2016-03-02 22:52:39 UTC
Didn't apply cleanly - I know because of the moving target that master is.  Please rebase.  Once it's done - leave me an @later in #koha and I'll jump right on it.  Thank you so much for the time and bother of rebasing.

Brendan
Comment 25 Jonathan Druart 2016-03-03 08:31:41 UTC
(In reply to Brendan Gallagher from comment #24)
> Didn't apply cleanly - I know because of the moving target that master is. 
> Please rebase.  Once it's done - leave me an @later in #koha and I'll jump
> right on it.  Thank you so much for the time and bother of rebasing.
> 
> Brendan

Brendan, we just need the last patch, others are already pushed.
Comment 26 Brendan Gallagher 2016-03-03 20:37:34 UTC
Ah thanks Jonathan - I thought it was something silly like that.  Ok last patch pushed to master - may 2016 release target.
Comment 27 Marcel de Rooy 2016-04-21 07:21:28 UTC
The unit test BiblioFrameworks.t does not have a transaction!
This is corrected with a patch under report 16155.