https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |julian.maurice@biblibre.com | |, | |kyle@bywatersolutions.com, | |m.de.rooy@rijksmuseum.nl, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Yes, and no. You can only do that if you know the patron has a guarantor, which is not always true. You must do: sub guarantor { my ( $self ) = @_; my $guarantor_rs = $self->_result->guarantorid; return unless $guarantor_rs; return Koha::Patron->_new_from_dbic($guarantor_rs); } I am definitely in favor of that, we should have a guideline about it IMO. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- We have already everything in our code: Koha::Item->biblio 93 sub biblio { 94 my ( $self ) = @_; 95 my $biblio_rs = $self->_result->biblio; 96 return Koha::Biblio->_new_from_dbic( $biblio_rs ); 97 } Koha::Item->checkout 121 sub checkout { 122 my ( $self ) = @_; 123 my $checkout_rs = $self->_result->issue; 124 return unless $checkout_rs; 125 return Koha::Checkout->_new_from_dbic( $checkout_rs ); 126 } Koha::Item->current_holds 271 sub current_holds { 272 my ( $self ) = @_; 273 my $attributes = { order_by => 'priority' }; 274 my $dtf = Koha::Database->new->schema->storage->datetime_parser; 275 my $params = { 276 itemnumber => $self->itemnumber, 277 suspend => 0, 278 -or => [ 279 reservedate => { '<=' => $dtf->format_date(dt_from_string) }, 280 waitingdate => { '!=' => undef }, 281 ], 282 }; 283 my $hold_rs = $self->_result->reserves->search( $params, $attributes ); 284 return Koha::Holds->_new_from_dbic($hold_rs); 285 } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Katrin Fischer from comment #4)

Do you still want to propose a coding guideline here, maybe for next dev meeting?

I believe we should. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- DRAFT: Within PERL15 under Koha::Object Koha::Object relationship accessors should be written to allow 'prefetch' For example one should use: sub guarantor { my $self = shift; my $guarantor_rs = $self->_result->guarantorid; return unless $guarantor_rs; return Koha::Patron->_new_from_dbic($gaurantor_rs); } One should not use: sub guarantor { my $self = shift; return Koha::Patrons->find( $self->guarantorid() ); } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |22684 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22684 [Bug 22684] Make Acquisition::Basket->bookseller match the code guidlines -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |22686 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22686 [Bug 22686] Koha::Acquisition::Bookseller->contacts should match the coding guidline -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |22700 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 [Bug 22700] Make biblio_metadata prefetchable from Koha::Biblio -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This has me wondering.. we're often renaming relationships within Koha::Objects (understandably as often our db derived ones are horrible because of poor initial db design). Should this guideline also introduce a requirement to match the relation name in the dbic class up to the relation name in the Koha::Object? It wouldn't be hard to add relations with such names and would lead to cleaner, easier to guess, prefetch strings. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- We could also use 'skip_relationships' in our dbicdump script to prevent outputting superfluous relationships given we can't/won't use them until we've manually written code in the corresponding Koha::Object class.. this way we would end up being much more explicit about what relationships were building (and as such reduce the load time/run time memory use form building massive dbic object full of unreachable expensive relationship accessors. That.. or we could start using the dbic recommended way of doing db work and go from schema to database rather than the other way around 😉 and ditch the script entirely. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Bug 22407 depends on bug 22700, which changed state. Bug 22700 Summary: Make biblio_metadata prefetchable from Koha::Biblio https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_20_05_target -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Bug 22407 depends on bug 22684, which changed state. Bug 22684 Summary: Make Acquisition::Basket->bookseller match the code guidlines https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22684 What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |RESOLVED Resolution|--- |WONTFIX -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_05_candidate |rel_20_11_target -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #13 from David Cook <dcook@prosentient.com.au> --- Honest question: Why do this: sub guarantor { my $self = shift; my $guarantor_rs = $self->_result->guarantorid; return unless $guarantor_rs; return Koha::Patron->_new_from_dbic($gaurantor_rs); } Instead of this: sub guarantor { my $self = shift; my $guarantor; my $guarantor_rs = $self->_result->guarantorid; if ($guarantor_rs){ $guarantor = Koha::Patron->_new_from_dbic($gaurantor_rs); } return $guarantor; } I admit that the latter has 3 more lines than the former, but the latter is way easier to read, easier to debug, and easier to add error-handling. Actually, that latter should probably double-check the output of Koha::Patron->_new_from_dbic() before returning. I've noticed a lot of Koha code directly returning the output of a method call, and it drives me crazy. We should do more data validation. It'll mean fewer bugs and easier maintenance. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #14)

What is there to check in this particular situation ? _new_from_dbic either dies or return a blessed object. If it dies then Koha::Patron::guarantor should die too, if it returns a blessed object, then there is nothing to check.

That's an interesting perspective. If it dies, it's probably going to bring down that entire Plack worker. That seems problematic to me. We could try to trap the error in the caller, but in practice that is rare in Koha. That being said, even if it doesn't die, we need to handle a non-fatal exception too. Maybe it is better to die in the grand scheme of things, as it'll more easily show code bugs and bad data. If it doesn't die, what happens if you want to add a check to that object? Maybe you only want to return the guarantor under certain conditions. It probably makes sense to add that check here rather than in every caller of this method. To do that, you'd have to refactor to use the syntax I described anyway. Or maybe you're doing some debugging because the guarantor being returned isn't correct. You can't add any logging in this code unless you refactor to the syntax I described. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #16)

In my opinion "maybe we will want to add something here" is not a good reason to refactor code. If that "maybe" ever comes true, then you can refactor the code while making the change. No need to do it now.

Given the number of legacy projects I've maintained over the years, we'll just have to agree to disagree on that one ;).

Note that I also tend to prefer your version, but I'm happy with both. If it ain't broke don't fix it ;)

True enough. There are better hills to die on heh. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_11_target |rel_21_05_candidate --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- We lost our way here. I updated PERL15 and marked it a defacto rule. Is anyone interested in moving this on for 21.05? I will help them as much as I can. Just let me know! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_11_candidate |rel_22_05_candidate -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.