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

Fix #3951 Bank NPC transfer errors #3955

Merged
merged 1 commit into from
Mar 12, 2022
Merged

Fix #3951 Bank NPC transfer errors #3955

merged 1 commit into from
Mar 12, 2022

Conversation

Znote
Copy link
Member

@Znote Znote commented Feb 18, 2022

Pull Request Prelude

Changes Proposed

Resolves 2 errors in bank NPC when trying to transfer money to another player.

  • If you specified a word instead of a number, NPC would throw a console error. Now it will repeat the question and ask specifically for a number.
  • If you specified a player name that didnt exist, NPC would throw a console error. Now it will inform user that it failed to find anyone with that name, and that he should make sure the name is correct.

Issues addressed:
#3951

@EPuncker EPuncker linked an issue Feb 18, 2022 that may be closed by this pull request
2 tasks
@EPuncker EPuncker mentioned this pull request Feb 18, 2022
2 tasks
@@ -233,6 +233,12 @@ local function creatureSayCallback(cid, type, msg)
end
end
elseif npcHandler.topic[cid] == topicList.TRANSFER_PLAYER_GOLD then
local currencyValue = tonumber(msg)
Copy link
Member

Choose a reason for hiding this comment

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

Better off making sure getMoneyCount returns a numeric value that is acceptable rather than adding checks like this (which may be required in other places as well).

@@ -248,6 +254,11 @@ local function creatureSayCallback(cid, type, msg)
end
elseif npcHandler.topic[cid] == topicList.TRANSFER_PLAYER_WHO then
transfer[cid] = getPlayerDatabaseInfo(msg)
if not transfer[cid] then
Copy link
Member

Choose a reason for hiding this comment

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

You may want to add this if-statement to L210 as well as removing the redundant if transfer[cid] then in both places.

@luanluciano93
Copy link
Contributor

luanluciano93 commented Feb 18, 2022

Can check that way @Znote? https://pastebin.com/raw/zKNLKi7z

@Znote
Copy link
Member Author

Znote commented Feb 23, 2022

I'm sure there is lots of refactoring and improvements to be made to NPC scripts and the associated lib functions, but this PR specifically is made to resolve 2 errors currently present within the bank NPC.

I'm not in the mood for refactoring, but feel free to push commits to the bankfix branch on Znote/forgottenserver to help me with improvements and further fixes.

@luanluciano93

Can check that way @Znote? https://pastebin.com/raw/zKNLKi7z

That code does not resolve the first error specified in my changes proposed list.
And why are you using pastebin in the land of git? 😄

@luanluciano93
Copy link
Contributor

Excuse my ignorance, but I don't know how to create a complete file review, nor a new pull request ...

I believe that this way would solve all the checks of the function getMoneyCount()

https://pastebin.com/0xqsmCnR

@DSpeichert DSpeichert merged commit 11fac4c into otland:master Mar 12, 2022
Codinablack pushed a commit to Codinablack/forgottenserver that referenced this pull request Apr 5, 2022
EPuncker pushed a commit to EPuncker/forgottenserver that referenced this pull request May 23, 2023
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.

Error in script bank.lua
5 participants