-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
lua/ps2/client/tabs/management_tab/manage_items/content/cl_pointshop2content.lua
Outdated
Show resolved
Hide resolved
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( ) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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>
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.
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.