Before patch you will see in acqui/booksellers.pl all the baskets ever created.
After the patch you will see in acqui/booksellers.pl only the basket with expected items.

Test plan :
* Create a basket with some orders lines
You should see this basket in acqui/booksellers.pl
* receive or cancel all the line in this basket
This basket shouldn't appear any more in acqui/booksellers.pl

Hello Lyon3
I did not test your patch yet, but a solution to "death by overbasketing" could also be hidding baskets linked to non active budgect.
And the same could be done for basketgroups too.
What is your opinion ?

M. Saby

After receiving all items, we still see the basket in booksellers.pl.

tested on sandbox biblibre

In reply of comment 3 : Mathieu I don't understand your "death by overbasketing" (we should talk about this yesterday...)
For basketgroups, we can do another patch...
Sonia

It works. But how can you find  a basket ?

You can search for baskets using the order search.

I had no time looking for it, but I think this is actually fixing a regression. It used to be so that you won't see any baskets that had been fully received in the vendor result list. Then that got changed again. I am thinking this could be a bug fix, at least for libraries coming from 3.6 and maybe a bit later.

I'm anticipating some questions from our librarian colleagues. They might ask us how to see all baskets from a vendor. I think it would be difficult for them to use acquisition module.
If all receveid baskets disappear from result list, it is good (cf desciption). But, with this patch, we can't see any baskets when we are on a vendor page.

It's always booksellers.pl which is called whatever you go in result list or you choose "baskets" in vendor page.
In order search, you can search by vendor and you will find all the orders with the name of basket...
The BZ9806 will permit to see the baskets in each basket group : it's an other way to find baskets from a vendor.

*** Bug 8611 has been marked as a duplicate of this bug. ***

I think there's something wrong with attachments:
the #2 is "octet-stream", it should be a patch I think.
isn't the #1 obsoleted by the #2 too ?

To answer the main question : I think it's more a bugfix than an ENH : baskets that are 100% received are supposed not to be displayed anymore on this page.

Side comment: we must display baskets that are not closed, whatever the # of items still to receive. Otherwise, empty baskets will disappear.

This patch doesn't apply for me through git-bz, but did apply using "git am -i3u" after saving the patch. I just wanted to mention it if others are testing too.

It works correctly to hide baskets with no items to be received, but I have one worry: Are librarians going to be confused about how to access those baskets? Should there be a "show all" link or something like that?

(In reply to comment #11)
> I'm anticipating some questions from our librarian colleagues. They might
> ask us how to see all baskets from a vendor. I think it would be difficult
> for them to use acquisition module.

Whoops, I see someone else has asked this already! I'm not a regular acquisitions user, so it's hard for me to say, however, I would think a "show all" link would be useful? Anyone else?

Hi Owen,

I'd really like to see the regression here fixed so that we are only showing baskets with outstanding orders by default, but a link to show more would certainly not hurt. Maybe we could just link into the aquisition search?

Hi Owen, reading your comments I see you tested this - is this ready for sign off or is there a problem?

In my opinion,

Sorry, Firefox is playing with me this morning...

So, in my opinion, the goal of Lyon 3 patch is good - showing only useful information in basket lists - , but I am not sure to like the way it is working.

The patch changes the default behavior GetBasketsInfosByBookseller function in Acquisition.pm. Would not it be better to maintain its default behavior, and to add an option if we want to filter the results ? (and in that case, the documentation of the function needs to be changed too).
Maybe we could also imagine a way for this function to not get by default baskets with documents only linked with non active budgets ?

But I think I would prefer keeping the function unchanged, and only hide received baskets in the templates. It will make possible the creation of a link "See also received basket" that would unhide received baskets. And I agree with Pierre : we must have a way to show all baskets if we want.
In templates, we could have checkboxes or option buttons to see received/pending/closed/open baskets, according to the whish of the librarians.

M. Saby

Hm I have done some research. The change discussed here was made when the result list for vendors was redone and GetBasketsInfosByBookseller was introduced (http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=d09e428a7dc9854c6f4e45f3caade3c8e141b79d):

The old list of baskets was created using following condition:
-where booksellerid = ?
-AND ( aqorders.quantity > aqorders.quantityreceived OR quantityreceived IS NULL)
-AND datecancellationprinted IS NULL

The new sub uses:
+        WHERE booksellerid = ?

The change proposed in the patch just restores the original condition:
+        WHERE booksellerid = ? 
AND ( aqorders.quantity > aqorders.quantityreceived OR quantityreceived IS NULL)
+         AND datecancellationprinted IS NULL

I know that the patch was pushed a while ago, but since we are still using 3.6.x in some libraries, this means a major change for them. Is there a chance we can agree on a solution here?

I am in favor of restoring the original behaviour for now and using the search to find other older baskets.

QA: Looking at this one now..

QA Comment:
Works fine. My only question was: How to find back this basket if it is no longer on that list. But with Order search or its Advanced Search, you can still find it.
Small patch. No complaints from qa-tools. Looks good.

BTW: I do not think that quantityreceived could be NULL (constrained), but this test does not hurt either.

Final thought: Perhaps another improvement could be: always show baskets that are still open and apply your condition only on the closed ones..
If you agree on that, could you add a followup?

Passed QA

Created attachment 18184 [details] [review]
Bug 9824 - Hide basket with no expected items in basqket list by bookseller

Before patch you will see in acqui/booksellers.pl all the baskets ever
created.
After the patch you will see in acqui/booksellers.pl only the basket
with expected items.

Test plan :
* Create a basket with some orders lines
You should see this basket in acqui/booksellers.pl
* receive or cancel all the line in this basket
This basket shouldn't appear any more in acqui/booksellers.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

This patch has been pushed to master and 3.12.x.

I have serious reservations about this patch, but am pushing it anyway as it simply reverts Koha to its previous behavior, problematic as that was. With this patch, I am no longer able to get an overview of baskets for a particular vendor. I can see a list of orders using the Orders search, but there's no overview of baskets available. I am pushing this only because I trust that someone will fix this issue properly for the 3.12.1 release.

I have been thinking about this for a while now and I can see that it's problematic because the old behaviour has been broken quite a while ago and now it could be a problematic change for other libraries using Koha's acq. 

I was wondering if I could use jQuery to 'revert' to the original behaviour... probably a bit tricky, but it could work until we see this fixed properly.

For 3.12.1, what about a filter on the left of the page, giving ability to choose to display received/pending/closed/open baskets, according to the whish of the librarians ?

M. Saby

That sounds nice - as long as we get something. I think having the shorter list (limited to 'unfinished' baskets) might be a good default, but have an option to show all/pending/open/closed baskets would certainly be nice.

(In reply to comment #27)
> I have been thinking about this for a while now and I can see that it's
> problematic because the old behaviour has been broken quite a while ago and
> now it could be a problematic change for other libraries using Koha's acq. 
> 
> I was wondering if I could use jQuery to 'revert' to the original
> behaviour... probably a bit tricky, but it could work until we see this
> fixed properly.

That would be quite easy to do:
$('.baskets tbody tr').each(function () {
    if ($('td:eq(2)', this).text() == $('td:eq(4)', this).text() && $('td:eq(2)', this).text() != '0') {
        $(this).hide();
    }
});

It sounds like a better patch is in the works for 3.12.1, so I am inclined to revert this patch for now, as it will change the behavior for anyone already using 3.8 or 3.10, and no one who upgraded from 3.6 noticed the change until this March. I will delay a decision until later today.

(In reply to comment #26)
> This patch has been pushed to master and 3.12.x.
> 
> I have serious reservations about this patch, but am pushing it anyway as it
> simply reverts Koha to its previous behavior, problematic as that was. With
> this patch, I am no longer able to get an overview of baskets for a
> particular vendor. I can see a list of orders using the Orders search, but
> there's no overview of baskets available. I am pushing this only because I
> trust that someone will fix this issue properly for the 3.12.1 release.

Am I missing something ?
Koha > Acquisitions > Order search > Advanced search > enter basket # in basket field, and you'll get all the content of basket # !
Doesn't that fit your need ?

Which use case could achieve a list of baskets for a particular vendor ? I don't see one (but I'm OK to be convinced being wrong ;-) )

(In reply to comment #31)
> (In reply to comment #26)
> Am I missing something ?
> Koha > Acquisitions > Order search > Advanced search > enter basket # in
> basket field, and you'll get all the content of basket # !
> Doesn't that fit your need ?

Not smoothly.

> Which use case could achieve a list of baskets for a particular vendor ? I
> don't see one (but I'm OK to be convinced being wrong ;-) )

I have a library that uses Acquisitions to record gifts, and each gift is a separate basket, and with a list of baskets they can review how often/when they received gifts, without needing to slog through lists of items.

"no one who upgraded from 3.6 noticed the change until this March"
I can say that my colleagues noticed and come to complain very often !!
I know that Lyon2 university had to change all the vendor every year to get around this problem.
I take account of the desire of people who manage the acquisitions in my university, but if operations differ in other organizations, i can understand that we try to giving ability to choose to display received/pending/closed/open baskets.
What is the need of people that really deals with acquisitions ?

I'm sorry but using acquistions module for gift is a sort of diversion, no ?
If this change is introduced not for the proper use, i'm not sure it's a good idea..
Sonia

In Rennes 2, we agree the default behavior MUST be changed, and we are fine with this patch being pushed in master.

The only remark is that MAYBE it could be useful for a lib, or the chief librarian responsible for acquisitions to see all baskets. But not everyday ;-)
And we could add new possibilities for filtering and sorting baskets (for ex. sorting by date, showing only open baskets etc).

Mathieu

Default should probably include the open baskets too, that could be excluded in the current change (as noted in the QA comment) if they have no order lines or if it is reopened but everything is received.

A simple link to remove this default filter (and thus show all) might be a way to prevent the need of reverting this patch?

This *trick* would not really involve a string change (for emphasis).

I would be satisfied with that change.

Still found a little issue with my change: sometimes supplier contains the search string, but sometimes the bookseller id numeric already.

Will have a quick look :)

(In reply to comment #40)
> Still found a little issue with my change: sometimes supplier contains the
> search string, but sometimes the bookseller id numeric already.

Resolved: added both ways in the url.

My last action for today on Bugzilla will be: eliminate this typo basqket in the title of this great report :)

Good news - you really tricked the translation scripts!
Now it would be perfect if we could make a link 'Apply filter' show up after clicking the link for the first time... looking into it.

Created attachment 18219 [details] [review]
[SIGNED OFF] Bug 9824: Followup for removing filter

Adds allbaskets parameter to GetBasketsInfosByBookseller. (Only used in booksellers.pl now)
Normally, all 'active' baskets are shown. With allbaskets=1 all baskets :)
In the template I had to rename a loop var supplier to supplier1 to resolve
name conflict between template vars.
In the template I added the string: Cancel filter.
Note that this string is already translated:
msgid "Cancel filter"
msgstr ""
Hope this helps.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Undoing the filter works and I checked that the string gets
translated with the po files in current master.
So this is almost perfect, only we can't apply the filters
again and the link remains 'cancel' when we already did.
Sending a follow-up trying to fix this.

Created attachment 18221 [details] [review]
[SIGNED-OFF] Bug 9824: Make it possible to apply/cancel switch

Using another existing string 'Apply filter' we are now
able to cancel the filter... and apply it... and cancel it
again... and so on.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Exercising my powers as RM and passing QA since the patches work.

This patch has been pushed to master and 3.12.x.

Katrin: Do you still have a suggestion on the second followup for textual changes? Perhaps from translation point of view. I submitted an amendment to make translation somewhat easier.

Status back to Signed off for two trivial QA followups.

As requested, back to Needs signoff.
Could a native speaker look at the proposed textual change?
Thanks.

Sandbox setup by unknown with database 2 and bug 9824 on Thu May 23 09:59:56 2013

Something went wrong !
Applying: Bug 9824: Followup for POD lines of GetBasketsInfosByBookseller
Applying: Bug 9824: Followup adding some text around Apply/cancel filter.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/booksellers.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/booksellers.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 9824: Followup adding some text around Apply/cancel filter.
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 9824 - Hide basket with no expected items in basket list by bookseller

Bug 9824: Followup for POD lines of GetBasketsInfosByBookseller
Apply? [yn]
Bug 9824: Followup adding some text around Apply/cancel filter.
Apply? [yn]
Patch left in /tmp/Bug-9824-Followup-adding-some-text-around-Applycan-myQQFX.patch

(In reply to comment #55)
> Sandbox setup by unknown with database 2 and bug 9824 on Thu May 23 09:59:56
> 2013
> 
> Patch failed at 0001 Bug 9824: Followup adding some text around Apply/cancel
> filter.

Thanks for testing!
Strange. These two patches do apply fine for me on current master.
Are you sure that the sandbox setup is on current master?

Back to Needs Signoff

Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>

Error sandbox for sinagture. I will revalidate the good "Pelardon"

Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>

Thank you for your quick signoff, cedric vita.
I reworded the text of the links on the second patch, with thanks to Owen.

The status was not changed. Could be marked as "Signed off" I suppose?

M. Saby

I think probably the reworded patch waits for sign-off.

ok, i'll test it.
but there is only 2 followups. Where is the main patch?

Mathieu

(In reply to comment #66)
> ok, i'll test it.
> but there is only 2 followups. Where is the main patch?
> 
> Mathieu

It was pushed to master and 3.12 way back on comment 49, at that point a new bug should have been created for any followups, because as you demonstrate, it is just confusing otherwise.

Remember people, once something is marked pushed to master, please to go back to needs signoff, (unless it is the patch rebased for a stable branch or something).

Thanks

(In reply to comment #66)
> but there is only 2 followups. Where is the main patch?

The second followup only needs a signoff, preferably from a native English speaker. (Note that I already followed the suggestion of Owen here.)
Thanks.

(In reply to comment #67)
> Remember people, once something is marked pushed to master, please to go
> back to needs signoff, (unless it is the patch rebased for a stable branch
> or something).

Not completely clear what you mean here, Chris. But these followups imo do not need a separate report..

Hm, I was tempted to suggest a separate bug too when I took a look at this. It seems like a good practice to me to use a new bug once the original feature has been pushed. And in this case the original feature has even been released :) - I am also thinking about the automatically created release notes here. Maybe something like "Reword basket filter in vendor search result list" would be more clear?

For testers and QA it's also harder to deal with a loooong discussion like this :)

(En réponse au commentaire 70)
> Hm, I was tempted to suggest a separate bug too when I took a look at this.
> It seems like a good practice to me to use a new bug once the original
> feature has been pushed. And in this case the original feature has even been
> released :) - I am also thinking about the automatically created release
> notes here. Maybe something like "Reword basket filter in vendor search
> result list" would be more clear?
> 
> For testers and QA it's also harder to deal with a loooong discussion like
> this :)

I don't know if it is already an enforced "rule", but 1 or 2 times I tried to add followup to bugs pushed to master, and I was said to create new bugs (for ex., a followup for UNIMARC of a MARC21 enh).
After having thougth at this, I believe it was a good advice ;-)

M. Saby

Sorry, will try to remember your suggestion next time :)
Katrin: As you suggested, I asked Owen on irc and adjusted the text. Would you mind passing these two patches on this report now or still require a new one?
Thanks.

Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>

All 3 patches of this bug have been pushed to master and 3.12 branches

(In reply to comment #75)
> All 3 patches of this bug have been pushed to master and 3.12 branches

No, this is not true. Somehow you obsoleted the two followups.
I will try to recover them now.

The two followups 18332 and 18506 are now signed off. They apply fine to current master.
Although I wrote them, they are actually QA followups on the three patches pushed before.
Since they only pertain to a small textual change and some POD lines, I am setting the status to Passed QA myself, herewith ending the confusion about the status of this report.

Passed QA

(In reply to comment #76)
> (In reply to comment #75)
> > All 3 patches of this bug have been pushed to master and 3.12 branches
> 
> No, this is not true. Somehow you obsoleted the two followups.
> I will try to recover them now.

And now I see the move of these patches to another report, aaaargggghhh.
OK. Please add a comment if you do so.
Will obsolete everything again..

Restored the situation now where Jonathan left it.
Learned something more about Bugzilla :)
The two followups on these three reports are on bug 10381.

(In reply to comment #79)
> The two followups on these three reports are on bug 10381.
reports -> patches

No confusion anymore now?

Sorry Marcel, I should have add a comment in addition to the "see also" field.

(In reply to comment #81)
> Sorry Marcel, I should have add a comment in addition to the "see also"
> field.

No problem. Just looking a little bit better could have helped me :)

Is it possible to push this patch to 3.10.x ?

Pushed to 3.10.x, will be in 3.10.7

Pushed to 3.8.x, will be in 3.8.14

(En réponse au commentaire 84)
> Pushed to 3.10.x, will be in 3.10.7

Thanks !