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

Duplicate license check: By MrCrowley #1035

Closed
wants to merge 5 commits into from

Conversation

gononono64
Copy link

Description

Duplicate license check just...doesnt work
Because of that this happens: https://www.youtube.com/watch?v=U1Pxpxuj7-o
If you bother to look into this you will realize you should merge this pretty much asap

PS I got lazy and rewrote what i did through the github editor. Its just a few minor modifications that i did. I also did these modifications on another server that was having the same issue. Person who brought it to my attention.

Please make sure i didnt miss anything. AKA make sure to test it

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.

dont need string.find
You really should get your shit together with the typos
removing whitespace
Linter bitching at me for yalls mess
@gononono64
Copy link
Author

There, next time dont approve things the linter doesnt accept...her der

@gononono64 gononono64 changed the title Duplicate license check: By MRCrowley Duplicate license check: By MrCrowley Nov 12, 2023
@iboss21
Copy link
Contributor

iboss21 commented Nov 12, 2023

we personally tested it on our server and fixed it now. thanks to MrCrowley

Copy link
Contributor

@Sh3ldar Sh3ldar left a comment

Choose a reason for hiding this comment

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

tested and working the commands.lua and functions.lua part is not needed for merge

@gononono64
Copy link
Author

tested and working the commands.lua and functions.lua part is not needed for merge

commands is needed for linter, functions there was a redundant check

Copy link
Contributor

@ChristianBDev ChristianBDev left a comment

Choose a reason for hiding this comment

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

LGTM | Tested and it works

@Qwerty1Verified
Copy link
Contributor

What part of the original code stops it from working? Is it the retrieving of the licenses before waiting that's stopping it?

@ChristianBDev
Copy link
Contributor

What part of the original code stops it from working? Is it the retrieving of the licenses before waiting that's stopping it?

This is where it is stopping it,

-- if GetConvarInt('sv_fxdkMode', false) then
    --     license = 'license:AAAAAAAAAAAAAAAA' -- Dummy License
    -- end

    if not license then
        deferrals.done(Lang:t('error.no_valid_license'))
    elseif QBConfig.Server.CheckDuplicateLicense and QBCore.Functions.IsLicenseInUse(license) then

@Qwerty1Verified
Copy link
Contributor

Checking that convar should be doable, this is the normal documented usage though local fxdkMode = GetConvarInt('sv_fxdkMode', 0) == 1

@gononono64
Copy link
Author

Checking that convar should be doable, this is the normal documented usage though local fxdkMode = GetConvarInt('sv_fxdkMode', 0) == 1

For what purpose?

@Qwerty1Verified
Copy link
Contributor

Qwerty1Verified commented Nov 13, 2023

Its purpose is to allow the usage of the Cfx.re Development Kit (FxDK). The edit on line 50 of the events.lua file also seems unnecessary.

still works with this added in
@gononono64
Copy link
Author

Its purpose is to allow the usage of the Cfx.re Development Kit (FxDK). The edit on line 50 of the events.lua file also seems unnecessary.
QBConfig.Server.CheckDuplicateLicense is definitely needed as its checking the config as it should. The previous was a typo

@Qwerty1Verified
Copy link
Contributor

It was previously checking the config through QBCore.Config

@gononono64
Copy link
Author

It was previously checking the config through QBCore.Config

image
image

@Qwerty1Verified
Copy link
Contributor

QBCore.Config = QBConfig

QBCore.Config is a variable alias defined by QBConfig

@gononono64
Copy link
Author

QBCore.Config = QBConfig

QBCore.Config is a variable alias defined by QBConfig

Well i can tell you from experience, being a developer and having to figure out what links where without any search options, is simply bad design. Makes it much harder to decode. At this point take it or leave it

@Qwerty1Verified
Copy link
Contributor

It's usually just a matter of best practice, hence why getters and setters exist to prevent access to the original data structure. People write code differently, but this is usually the practice that is followed. When most of the other code uses QBCore.Config, it can be assumed that it works if it hasn't been modified, and then of course a quick search for QBCore.Config would reveal that it is a real variable and has a role

@gononono64
Copy link
Author

It's usually just a matter of best practice, hence why getters and setters exist to prevent access to the original data structure. People write code differently, but this is usually the practice that is followed. When most of the other code uses QBCore.Config, it can be assumed that it works if it hasn't been modified, and then of course a quick search for QBCore.Config would reveal that it is a real variable and has a role

image

@Qwerty1Verified
Copy link
Contributor

Yeah that's why I said different people write code differently and that most of the other code uses QBCore.Config. Simply trying to follow best practice, and that's just a few occasions where it's using QBConfig. I'm glad you found the cause of the bug though

@Qwerty1Verified
Copy link
Contributor

Has it been tested since the convar check was added back in? It'll be nice to see a bug like this fixed

@iboss21
Copy link
Contributor

iboss21 commented Nov 13, 2023

I m glad its fixed and servers owners as myself can be relaxed. Thank you guys and thanks to QBCore Community for this fast response and fixing it.

Truly appreciated.

@gononono64
Copy link
Author

gononono64 commented Nov 13, 2023

Has it been tested since the convar check was added back in? It'll be nice to see a bug like this fixed

yes. have not tested the convar itself tho

@GhzGarage
Copy link
Member

d7323ba

@GhzGarage GhzGarage closed this Nov 13, 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.

6 participants