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

Feat: Added money transfer functions #1099

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Cocodrulo
Copy link
Contributor

@Cocodrulo Cocodrulo commented Apr 18, 2024

Description

Implementation of the suggestion [SUGGESTION] Implement monetary transactions as a function. There have been added to two new functions. One in functions (QBCore.Functions.TransferMoney) and other one on player (Player.Functions.TansferTo), both aproach the same objective, trasnfering money betwwen two players if both are online, online one is online or both are offline.

Checklist

  • I have personally loaded this code into an updated qbcore project and checked all of its functionality.
  • My code fits the style guidelines.
  • My PR fits the contribution guidelines.

@MonsieurBibo
Copy link

Black money is not in the core, but crypto is, and you didn't put it in ?

@Cocodrulo
Copy link
Contributor Author

Good point, but actually that information is only commented information, I mean is not really whitelisting those monetypes it is only info not actual code.

Anyway I'll fixed it tomorrow

@Cocodrulo
Copy link
Contributor Author

Fixed the annotation and also renamed the Player.Functions.TransferTo to Player.Functions.TransferMoneyTo to make it self explainable.

Copy link

@Scartane Scartane left a comment

Choose a reason for hiding this comment

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

Some thing to change to prevent failure and I think code can be optimize a little bit (wording etc included)

---@param quant number
---@param reason string
---@return boolean
function QBCore.Functions.TransferMoney(emitercid, emitermoneytype, receivercid, receivermoneytype, quant, reason)

Choose a reason for hiding this comment

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

quant should be replaced by amount since it's the wording use by default for this kind of property

Copy link

@Scartane Scartane Jun 13, 2024

Choose a reason for hiding this comment

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

sourcecid and targetcid wil lbe better than emitercid / receivercid no ? and same for all properties then ? What do you think ?

if not result then errorOnLast = true end
result = json.decode(result)
result[receivermoneytype] += quant
if not MySQL.update.await('UPDATE players SET money = ? WHERE citizenid = ?', { json.encode(result), receivercid }) then return false end

Choose a reason for hiding this comment

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

you should not return false here but do errorOnLast = true or the emiter will loose the money if their is an error on the receiver player

@@ -337,6 +337,29 @@ function QBCore.Player.CreatePlayer(PlayerData, Offline)
return true
end

function self.Functions.TransferMoneyTo(emitermoneytype, receivercid, receivermoneytype, quant, reason)

Choose a reason for hiding this comment

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

quant should be replaced by amount since it's the wording use by default for this kind of property

local EmiterPlayer = QBCore.Functions.GetPlayerByCitizenId(emitercid)
local ReceiverPlayer = QBCore.Functions.GetPlayerByCitizenId(receivercid)
if not tonumber(quant) then return false end
quant = tonumber(quant) or 0

Choose a reason for hiding this comment

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

operation with 0 should not be done I think. Probably you can do if not tonumber(quant) or tonumber(quant) <= 0 then return false end or just if tonumber(quant) <= 0 then return true end

function self.Functions.TransferMoneyTo(emitermoneytype, receivercid, receivermoneytype, quant, reason)
local ReceiverPlayer = QBCore.Functions.GetPlayerByCitizenId(receivercid)
if not tonumber(quant) then return false end
quant = tonumber(quant) or 0

Choose a reason for hiding this comment

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

same remark at the first file regarding 0 transaction handling

if not result then errorOnLast = true end
result = json.decode(result)
result[receivermoneytype] += quant
if not MySQL.update.await('UPDATE players SET money = ? WHERE citizenid = ?', { json.encode(result), receivercid }) then return false end

Choose a reason for hiding this comment

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

instead of return false it should be errorOnLast = true

@Cocodrulo
Copy link
Contributor Author

Changed those things that you commented, you were right in all of them jajaja

@Scartane
Copy link

Changed those things that you commented, you were right in all of them jajaja

Will check that soon and let you know if I find something else

@Cocodrulo
Copy link
Contributor Author

Sorry I added changes by mistake, changes reverted

Copy link

@Scartane Scartane left a comment

Choose a reason for hiding this comment

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

LGTM 👍
I don't know if we can add some log, but regarding the code itself i don't see any other mistake :)

@Cocodrulo
Copy link
Contributor Author

Jus added logs and Money Events, a bit chunky the blocks of code but I don't know how to make it smaller.

Copy link

github-actions bot commented Nov 7, 2024

This PR has had 60 days of inactivity & will close within 7 days

@github-actions github-actions bot added the Stale label Nov 7, 2024
@Cocodrulo
Copy link
Contributor Author

Any update?

@github-actions github-actions bot removed the Stale label Nov 10, 2024
@GhzGarage
Copy link
Member

Feel like this functionality could just be added onto AddMoney and RemoveMoney

@GhzGarage GhzGarage self-assigned this Nov 13, 2024
@GhzGarage GhzGarage added enhancement New feature or request help wanted Extra attention is needed question Further information is requested Needs Testing/Reviews labels Nov 13, 2024
@GhzGarage GhzGarage linked an issue Nov 13, 2024 that may be closed by this pull request
@Cocodrulo
Copy link
Contributor Author

I don't understand that

@Cocodrulo
Copy link
Contributor Author

hello?

@Cocodrulo Cocodrulo changed the title 💱 Added money transfer functions Feat: Added money transfer functions Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed Needs Testing/Reviews question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SUGGESTION] Implement monetary transactions as a function
4 participants