[Koha-bugs] [Bug 25260] Merge 'reserves' and 'old_reserves' into a new 'holds' table

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25260

--- Comment #68 from Tomás Cohen Arazi <tomascohen at gmail.com> ---
(In reply to Jonathan Druart from comment #65)
> (In reply to Tomás Cohen Arazi from comment #63)
> 
> > - t/db_dependent/Reserves.t failure
> 
> There are 2 things:
> 1.
> 1107                 item_id          => $item->biblionumber,
> Key must be biblio_id
> 
> 2.
> The $title_level_target_query query in _Findgroupreserve is now returning
> the "reserved" hold. On master it's not matching any rows and the third
> query ($query) is hit and returned the different holds.
> 
> I think it's coming from:
> -        JOIN hold_fill_targets USING (reserve_id)
> vs
> +        JOIN hold_fill_targets ON (
> +                holds.biblio_id=hold_fill_targets.biblionumber
> +            AND holds.patron_id=hold_fill_targets.borrowernumber)

Thanks! That solved Reserves.t. Will ask Agustin about that change, because we
merged many commits in our branch into this commit (a year ago).

> > - t/db_dependent/Circulation.t failure
> 
>     #          got: 'on_reserve'
>     #     expected: 'too_soon'
> Related to hold's status as well so may be fixed if the previous test is
> corrected.

This might be something else because the problem stands. Will submit the
version with the fix for review.

> > - Add some warning in about.pl about wrong letters (maybe?)
> 
> After we moved the marcxml out of biblioitems we added a warning on the
> report list view. Maybe we should do the same for the notice templates?
> 
>   commit f22d2e7200ee8b35aff66b26acc3e2daa49f9f0d
>   Bug 17898: Automagically convert SQL reports

Good idea. If there are chances this dev gets pushed to master, this is
something I will work on.

> Questions:
> * Shouldn't *_date DB fields be *_on?

Probably, Some will be _on, some others _until. I didn't think about it and
went into doing the API-way. I didn't find a written Coding guideline about
this, though. No problem for me with changing this. It will require a bunch of
new mappings only (Koha::Hold->to_api_mapping).

> * item_level => item_level_request

Currently it is:
- 'item_level_hold' in (old)reserves
- 'item_level_request' in hold_fill_target and tmp_holdsqueue
- 'item_level' on the API (voted)

> I think we agreed on "item_level_request", why did you change it?

As with dates, I preferred to directly use the API version. Can be revisited in
a follow-up patch along with the other changes if QA requires it.

> * Shouldn't holds.id be holds.hold_id? I cannot remember when/where but I
> think we agreed we shouldn't use "id" (to prevent wrong id to be returned on
> JOIN).

That's correct, and it is in the guidelines. Follow-up coming.

Thanks for the review!

-- 
You are receiving this mail because:
You are watching all bug changes.