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

🔑 Optionally save acquired keys into DB #265

Merged
merged 11 commits into from
Nov 27, 2024

Conversation

Cocodrulo
Copy link
Contributor

Describe Pull request
This PR makes so the aquired keys will be saved into DB so when the server restarts you will still have the keys you were given. This feature is optional and can be set in the Config file. The PR is made to answer this suggestion [SUGGESTION] Database keys. I believe that is easier to make this into player metadata instead of a full new database table. Also this PR was already open, but also closed because of lack of answer, so I decided to make a new one: old one here.

Questions (please complete the following information):

  • Have you personally loaded this code into an updated qbcore project and checked all it's functionality? Yes
  • Does your code fit the style guidelines? Yes
  • Does your PR fit the contribution guidelines? Yes

Copy link

@Qwerty1Verified Qwerty1Verified left a comment

Choose a reason for hiding this comment

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

Instead of doing it from the client, handle it on the server since to give someone keys the server event gets triggered which also checks if they should be giving keys.

I'd also probably change the config value to be more something like Config.PersistentKeys although others might disagree. I'll ask others.

@Z3rio
Copy link
Contributor

Z3rio commented Nov 14, 2024

Quoted from my comment left Apr 18 in your past PR for this, #255.
Which for that matter, is exactly the same as this PR.

Lovely PR, and great idea.
But doing it this way is not recommended, as anyone could trigger that server event from their client with any kind of key list.

This should be something to be fully handled on the serverside if it should be secure, qb-core already has enough insecure server events as it is.

Also this PR was already open, but also closed because of lack of answer

Yes indeed, from your side though...
So opening an PR with the exact same files/changes will probably just generate the same result.

@Z3rio
Copy link
Contributor

Z3rio commented Nov 14, 2024

If you want pointers on how to improve it, I'll gladly assist

@Cocodrulo
Copy link
Contributor Author

This changes should do the trick

Copy link
Contributor

@Z3rio Z3rio left a comment

Choose a reason for hiding this comment

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

Overall great, much better approach this time! 👍🏼 🚀

Some of these "comments" might be a bit nitpicky, other ones quite blatant/more important.
Do as you wish with my feedback. Great work this far!

server/main.lua Outdated Show resolved Hide resolved
server/main.lua Outdated Show resolved Hide resolved
server/main.lua Outdated Show resolved Hide resolved
config.lua Outdated Show resolved Hide resolved
TriggerClientEvent('qb-vehiclekeys:client:RemoveKeys', id, plate)
end

exports('RemoveKeys', RemoveKeys)

function HasKeys(id, plate)
local citizenid = QBCore.Functions.GetPlayer(id).PlayerData.citizenid
local Player = QBCore.Functions.GetPlayer(id)
if not Player then return false end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if not Player then return false end
if not Player then
return false
end

better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is better to keep it one line, it's cleaner from my view

TriggerClientEvent('QBCore:Notify', id, Lang:t('notify.vgetkeys'))
TriggerClientEvent('qb-vehiclekeys:client:AddKeys', id, plate)
end

exports('GiveKeys', GiveKeys)

function RemoveKeys(id, plate)
local citizenid = QBCore.Functions.GetPlayer(id).PlayerData.citizenid
local Player = QBCore.Functions.GetPlayer(id)
if not Player then return end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if not Player then return end
if not Player then
return
end

better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is better to keep it one line, it's cleaner from my view

@Cocodrulo
Copy link
Contributor Author

Thanks for the ideas and corrections, comments like that are always welcome

@Cocodrulo
Copy link
Contributor Author

if this going to be added?

@GhzGarage GhzGarage merged commit db774ee into qbcore-framework:main Nov 27, 2024
2 checks passed
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.

4 participants