|
Description
Andrew Fuerste-Henry
2021-08-11 12:58:40 UTC
Looks like we need an additional link between the 2 tables aqorders and suggestions to achieve that.
That would make sense, especially when looking at such code:
SELECT suggestions.*,
U1.surname AS surnamesuggestedby,
U1.firstname AS firstnamesuggestedby,
U1.borrowernumber AS borrnumsuggestedby
FROM suggestions
LEFT JOIN borrowers AS U1 ON suggestedby=U1.borrowernumber
WHERE biblionumber=?
LIMIT 1
in GetSuggestionInfoFromBiblionumber. It is wrong if there are several suggestions on the same bib.
Hard to know it won't break existing workflows however.
(In reply to Jonathan Druart from comment #1) > Looks like we need an additional link between the 2 tables aqorders and > suggestions to achieve that. > > That would make sense, especially when looking at such code: > > SELECT suggestions.*, > U1.surname AS surnamesuggestedby, > U1.firstname AS firstnamesuggestedby, > U1.borrowernumber AS borrnumsuggestedby > FROM suggestions > LEFT JOIN borrowers AS U1 ON suggestedby=U1.borrowernumber > WHERE biblionumber=? > LIMIT 1 > > in GetSuggestionInfoFromBiblionumber. It is wrong if there are several > suggestions on the same bib. > > Hard to know it won't break existing workflows however. I agree - we should have a real link and not be guessing. Created attachment 151001 [details] [review] Bug 28844: DB changes Created attachment 151002 [details] [review] Bug 28844: DBIC schema changes Created attachment 151003 [details] [review] Bug 28844: DBIC specific Created attachment 151004 [details] [review] Bug 28844: Add link between suggestions and orders Created attachment 151005 [details] [review] Bug 28844: Remove GetSuggestionInfoFromBiblionumber Created attachment 151006 [details] [review] Bug 28844: Replace GetSuggestionFromBiblionumber Created attachment 151007 [details] [review] Bug 28844: Remove GetSuggestionFromBiblionumber Well, no test plan here. You need to test the whole suggestion/acquisition workflow and search for bugs! Link should be displayed correctly as before, and even better in cause of multiple suggestions on the same record. Please do not backport this. Andrew, Katrin, can I get your opinion on these patches please? :) *** Bug 17463 has been marked as a duplicate of this bug. *** Sorry, patch doesn't apply. Please reabase! Created attachment 153157 [details] [review] Bug 28844: DB changes Created attachment 153158 [details] [review] Bug 28844: DBIC schema changes Created attachment 153159 [details] [review] Bug 28844: DBIC specific Created attachment 153160 [details] [review] Bug 28844: Add link between suggestions and orders Created attachment 153161 [details] [review] Bug 28844: Remove GetSuggestionInfoFromBiblionumber Created attachment 153162 [details] [review] Bug 28844: Replace GetSuggestionFromBiblionumber Created attachment 153163 [details] [review] Bug 28844: Remove GetSuggestionFromBiblionumber Created attachment 153174 [details] [review] Bug 28844: Remove GetSuggestionInfoFromBiblionumber Created attachment 153175 [details] [review] Bug 28844: Replace GetSuggestionFromBiblionumber Created attachment 153176 [details] [review] Bug 28844: Remove GetSuggestionFromBiblionumber I'd really love to see this long standing issue fixed :)
1) Migration of data
At the moment this change would break processing the existing suggestions. As the suggestion_id for the existing orders is empty, the information would not show up on the orders in acq, and I am also not sure that the patrons would be informed by email.
My suggestion:
We only compare entries with the same biblionumber:
1 suggestion with status in (ORDERED, AVAILABLE) and 1 order => link them
1 suggestion with status (ORDERED) and 1 order with orderstatus in (ordered, partial) => link them
1 suggestion with status (AVAILABLE) and 1 order with orderstatus (complete) => link them
For the rest:
Mimick what we have done until now, use the first match (LIMIT 1, ->single):
For every order line that has no suggestion_id yet
that has a matching biblionumber in suggestions
link to the suggestion_id with the lowest/smallest suggestion_id where status in (ORDERED, AVAILABLE)
So it might not be correct, but I think it's what would be displayed on the order line now and where the email would be sent to, so that might be the best we can do?
2) ModSuggestion
ModSuggestion is called to update the status of the suggestion and to send the email, but the calls haven't always been adjusted to look up the correct suggestion.
Orders.pm cancel()
my $suggestion = Koha::Suggestions->find({ biblionumber => $self->biblionumber, status => "ORDERED" });
if ( $suggestion and $suggestion->id ) {
Wouldn't this fail if there is more than one suggestion for biblionumber with status ORDERED?
addorder.pl = seems OK, because using the suggestion_id passed, but double checking would be good
EDI.pm
my $s = $schema->resultset('Suggestion')->search(
{
biblionumber => $order->biblionumber->biblionumber,
}
)->single;
if ($s) {
ModSuggestion(
{
suggestionid => $s->suggestionid,
STATUS => 'AVAILABLE',
}
);
}
And there are more calls I haven't checked that need to be verified.
Created attachment 158982 [details] [review] Bug 28844: DB changes Created attachment 158983 [details] [review] Bug 28844: DBIC schema changes Created attachment 158984 [details] [review] Bug 28844: DBIC specific Created attachment 158985 [details] [review] Bug 28844: Add link between suggestions and orders Created attachment 158986 [details] [review] Bug 28844: Remove GetSuggestionInfoFromBiblionumber Created attachment 158987 [details] [review] Bug 28844: Replace GetSuggestionFromBiblionumber Created attachment 158988 [details] [review] Bug 28844: Remove GetSuggestionFromBiblionumber Created attachment 158989 [details] [review] Bug 28844: Adjust other calls Created attachment 158990 [details] [review] Bug 28844: Adjust tests (In reply to Katrin Fischer from comment #25) > I'd really love to see this long standing issue fixed :) > > > 1) Migration of data > So it might not be correct, but I think it's what would be displayed on the > order line now and where the email would be sent to, so that might be the > best we can do? It was left on purpose. I don't think we should copy an information if we are not sure it is correct. I am not confident in migrating something here. > 2) ModSuggestion > > ModSuggestion is called to update the status of the suggestion and to send > the email, but the calls haven't always been adjusted to look up the correct > suggestion. Correct, good catch. Should be fixed. > And there are more calls I haven't checked that need to be verified. Other calls look good. I let you decide what the status is. Created attachment 159759 [details] [review] Bug 28844: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159760 [details] [review] Bug 28844: DBIC schema changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159761 [details] [review] Bug 28844: DBIC specific Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159762 [details] [review] Bug 28844: Add link between suggestions and orders Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159763 [details] [review] Bug 28844: Remove GetSuggestionInfoFromBiblionumber Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159764 [details] [review] Bug 28844: Replace GetSuggestionFromBiblionumber Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159765 [details] [review] Bug 28844: Remove GetSuggestionFromBiblionumber Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159766 [details] [review] Bug 28844: Adjust other calls Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159767 [details] [review] Bug 28844: Adjust tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159768 [details] [review] Bug 28844: (follow-up) Adjust suggestion id comparison Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Added a minor follow-up, otherwise it seemed to work as expected, QA tool does note some untidiness I think the link here is wrong though. I believe the suggestions need to store the ordernumber - this allows for multiple suggestions on a single order. The current patches limit an order being tied to a single suggestion. If a library has two patrons suggest the same title, they may only want to order a single copy, or place a single order for multiple copies. Could it be done with the order number stored in suggestions? I think I understand what you mean, Nick, but the UI is not allowing it for now (to have several suggestions linked to an order). Should we really add complexity (at least rewrite everything here) in case maybe one day we will implement it? It will still be possible later to move the data. Maybe we could extend the relationship later. At the moment we create an order from a suggestion, so a 1:1 makes sense. When I tested this my main concern was to ensure that the suggestions existing previously to this fix would still work, patrons receiving emails etc. Did you take a look at that aspect? (In reply to Katrin Fischer from comment #48) > Maybe we could extend the relationship later. At the moment we create an > order from a suggestion, so a 1:1 makes sense. > > When I tested this my main concern was to ensure that the suggestions > existing previously to this fix would still work, patrons receiving emails > etc. Did you take a look at that aspect? Yes, but didn't fix it. Seen comment 35? (In reply to Jonathan Druart from comment #49) > (In reply to Katrin Fischer from comment #48) > > Maybe we could extend the relationship later. At the moment we create an > > order from a suggestion, so a 1:1 makes sense. > > > > When I tested this my main concern was to ensure that the suggestions > > existing previously to this fix would still work, patrons receiving emails > > etc. Did you take a look at that aspect? > > Yes, but didn't fix it. Seen comment 35? Yes, but ... I think we need to fix this, but I also think we can't break existing suggestions especially since it involves a service the library provides to their patrons and they can't really fix it in the GUI either. Too late today, but we should give this some more thought. I am not willing to write the DB rev to adjust the existing entries. I would like help here please. OK, I am not sure I could write it either, but the first step is to make a plan. I reworked this with the relationship built the other way on bug 35717 I can attempt the update there, however, I think a better path is to allow a GUI way for the suggestions to be correctly linked, or to attempt to update and provide a way to correct if needed. I will be working on 19358 and 31632 this cycle which will be part of the GUI. Please let me know if we can agree to continue this work on 35717 How does this bug relate to bug 35717? Both seem very similar, same issue too :( |