Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Refunds at purchase price and current price #194

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wgaylord
Copy link

@wgaylord wgaylord commented Dec 3, 2023

This fixes the refund system for deleting items and extends it,

It allows for refunding items at the purchase price (price at time the player purchased it)
OR
At current price of the item

When refunding at current price it uses the currency based on what currency was used to purchase it.

This requires the fix to LibK to be merged to actually work.

Copy link
Owner

@ValentinFunk ValentinFunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, cool that you're implementing this! Haven't been able to run it but added some code comments

if refundCurrentPrice then
currentPrice = table.Inherit(itemClass.static.Price, { points = 0, premiumPoints = 0 })
end
local refundsByPlayer = calculateRefundAmounts( refund, currentPrice )
-- Remove players that get refunded 0 points
local toRefund = LibK._( refundsByPlayer ):chain()
:entries( )
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the filter below have amountsToRefund = entry[2]? I think it's { ownerId, amountsToRefund } so same mistake you fixed below

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, although it is working this way, so I don't think it needs to be changed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don't have a gmod server to test, would be good to verify this, if entry[1] is actually the ownerId the could would try to do e.g. 123.points != 0 which should error - not sure why it doesn't since you tested this

if refundCurrentPrice then
currentPrice = table.Inherit(itemClass.static.Price, { points = 0, premiumPoints = 0 })
end
local refundsByPlayer = calculateRefundAmounts( refund, currentPrice )
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be calculateRefundAmounts( results, currentPrice )? Maybe the table.Inherit(itemClass.static.Price, { points = 0, premiumPoints = 0 }) part would be better to put into calculateRefundAmounts (pass in refundCurrentPrice boolean and use the current price or purchaseData depending on it)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is the itemClass does not get passed into calculateRefundAmounts (At least from what I found using MsgAll and stuff, allitle hard to debug the tables when PrintTable does nothing.)

If you can get the itemClass in it, then that would be a good idea.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're up for giving that a try I think it'd make sense - in the diff here I can only see that currentPrice is passed in to calculateRefundAmounts but it's not used. Am I missing something here it looks like refunding the current price shouldn't be able to work because the value doesn't make it all the way into the calculation 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I copied the wrong revision of my code when making this PR. Didn't want to push the whole file I developed this in since Dinks runs on an older version of PS2. (Due to some weird bug that also involves PAC3, not sure since I haven't seen it myself) I have pushed the info now. Going to re-do my testing again tho. Just to be sure it all works properly.

Co-authored-by: Valentin <funk.valentin@gmail.com>
@wgaylord
Copy link
Author

Any further comments?

@ValentinFunk
Copy link
Owner

ValentinFunk commented Dec 22, 2023

Any further comments?

Hey sorry for the delay here and thanks for the ping! Added some comments above

Going to re-do my testing to make sure it all still works now that I have the fully up to date version of the functions I implemented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants