alfio-event / alf.io

alf.io - The open source ticket reservation system for conferences, trade shows, workshops, meetups
https://alf.io
GNU General Public License v3.0
1.35k stars 341 forks source link

Fix user-defined donation prices being saved with zero value #1363

Closed shanebrowncs closed 3 weeks ago

shanebrowncs commented 1 month ago

Hi,

After updating my alf.io instance from 2.0-M4-2304 to 2.0-M4-2402-3 I noticed donation fields with user-defined pricing no longer work properly.

Here is an example of a ticket which costs $5.50 with an attached donation of $7.00: 2024-06-09_01

When I reach the summary screen you can see the donation value in the table is $0.00: 2024-06-09_02

Upon investigating I found this is being caused by the incorrect field being inserted into additional_service_item.src_price_cts. When the event/{eventName}/reserve-tickets endpoint is called, eventually we get to the 'AdditionalServiceManager.bookAdditionalServiceItems' method where we are inserting the additional_service_item rows to the database. For the src_price_cts column we are inserting as.getSrcPriceCts() which when the price is NOT fixed has a value of 0. When calling 'AdditionalServicePriceContainer.getSrcPriceCts' it already has it's own method of dealing with this issue by checking whether the price is fixed:

    @Override
    public int getSrcPriceCts() {
        if(additionalService.isFixPrice()) {
            return additionalService.getSrcPriceCts();
        }
        return Optional.ofNullable(customAmount).map(a -> MonetaryUtil.unitToCents(a, currencyCode)).orElse(0);
    }

Because of this it seemed appropriate to just substitute the call to as.getSrcPriceCts() with pc.getSrcPriceCts(). If you could please let me know if this is something you would be interested in merging. I am not very familiar with the codebase but would be happy to look at implementing further required changes if necessary.

Thank you.

syjer commented 4 weeks ago

Hi @shanebrowncs , thank you for noticing and opening a PR.

If it's possible, could you also add a test? (My guess a unit test would be enough)

shanebrowncs commented 4 weeks ago

Hi @syjer, I have added an additional commit with a new unit test file for AdditionalServiceManager. It tests both main code paths in bookAdditionalServiceItems(). Please let me know if this is acceptable.

Is there any chance this could be included in a patch release? I already have upgraded my instance to 2.0-M4-2402-3 and rely on this feature.

Thank you.

syjer commented 3 weeks ago

hi @shanebrowncs , everything looks fine, thank you for the PR! We will merge it, cherry pick on the m4 branch and do a patch release.

shanebrowncs commented 3 weeks ago

Great, thank you!

cbellone commented 3 weeks ago

merged. Thank you!