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 radio nui not updating and restricted channels #47

Merged
merged 13 commits into from
Nov 12, 2024

Conversation

Cocodrulo
Copy link
Contributor

Describe Pull request
Fixed the problem where restricted channels where not checked when connecting through NUI and radio HUD not updating the channel number when increasing, decreasing or changin channel. Also fix issue [BUG] #46

Questions (please complete the following information):

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

@danichim
Copy link

Is not updating the channel when you increase from UI buttons

Copy link
Contributor

@mbiddle mbiddle left a comment

Choose a reason for hiding this comment

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

Good changes overall. Broken until the access typo is fixed but I suggested a few other changes.

@@ -57,15 +65,23 @@ $(document).on('click', '#decreaseradiochannel', function(e){

$.post('https://qb-radio/decreaseradiochannel', JSON.stringify({
channel: $("#channel").val()
}));
})).then((data) => {
if (data.canacces) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be data.canaccess

});

$(document).on('click', '#increaseradiochannel', function(e){
e.preventDefault();

$.post('https://qb-radio/increaseradiochannel', JSON.stringify({
channel: $("#channel").val()
}));
})).then((data) => {
if (data.canacces) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be data.canaccess

@@ -27,7 +31,11 @@ $(document).on('click', '#submit', function(e){

$.post('https://qb-radio/joinRadio', JSON.stringify({
channel: $("#channel").val()
}));
})).then((data) => {
if (data.canacces) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be data.canaccess

@@ -15,7 +15,11 @@ $(function() {
} else if (data.key == "Enter") { // Enter key
$.post('https://qb-radio/joinRadio', JSON.stringify({
channel: $("#channel").val()
}));
})).then((data) => {
if (data.canacces) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be data.canaccess

@@ -29,6 +29,12 @@ local function SplitStr(inputstr, sep)
end

local function connecttoradio(channel)
if Config.RestrictedChannels[channel] ~= nil then
Copy link
Contributor

Choose a reason for hiding this comment

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

The up and down channel functions don't respect the min/max. We should also move the min/max channel check here and remove it from joinRadio.

if channel > Config.MaxFrequency or channel <= 0 then QBCore.Functions.Notify(Lang:t('restricted_channel_error'), 'error') return false end

client.lua Outdated
QBCore.Functions.Notify(Lang:t("increase_decrease_radio_channel", {value = newChannel}), "success")
cb("ok")
local canaccess = connecttoradio(newChannel)
if canaccess then QBCore.Functions.Notify(Lang:t("increase_decrease_radio_channel", {value = newChannel}), "success") end
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this double notification as it's already handled in the connecttoradio function.

client.lua Outdated
QBCore.Functions.Notify(Lang:t("increase_decrease_radio_channel", {value = newChannel}), "success")
cb("ok")
local canaccess = connecttoradio(newChannel)
if canaccess then QBCore.Functions.Notify(Lang:t("increase_decrease_radio_channel", {value = newChannel}), "success") end
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this double notification as it's already handled in the connecttoradio function.

@Cocodrulo
Copy link
Contributor Author

Thanks for the suggestions, all done

Copy link
Contributor

@mbiddle mbiddle left a comment

Choose a reason for hiding this comment

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

Changes look good on my end. I have a few final pieces of feedback.

One unrelated piece is I was working on a similar PR that I will close now that I see you are back and active, but one remaining change to qb-radio would be to update the fxmamnifest.lua file to include both radio images. Please see my changes here and bring them into this branch. Thanks!

client.lua Outdated
@@ -151,15 +159,11 @@ RegisterNUICallback('joinRadio', function(data, cb)
if rchannel ~= nil then
if rchannel <= Config.MaxFrequency and rchannel ~= 0 then
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not getting this the first time around, but I think we should force rchannel to to be greater than 0 here. Do we really want negative channels?

if rchannel <= Config.MaxFrequency and rchannel > 0 then

end)

RegisterNUICallback("decreaseradiochannel", function(_, cb)
if not onRadio then return end
local newChannel = RadioChannel - 1
if newChannel >= 1 then
connecttoradio(newChannel)
QBCore.Functions.Notify(Lang:t("increase_decrease_radio_channel", {value = newChannel}), "success")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good change, but please remove the old increase_decrease_radio_channe phrase in the locales files.

@Cocodrulo
Copy link
Contributor Author

Done all changes, thanks for your input

Copy link
Contributor

@mbiddle mbiddle left a comment

Choose a reason for hiding this comment

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

Not that I really matter here, but the code looks good from my end!

client.lua Outdated
@@ -29,6 +29,13 @@ local function SplitStr(inputstr, sep)
end

local function connecttoradio(channel)
if channel > Config.MaxFrequency or channel <= 0 then QBCore.Functions.Notify(Lang:t('restricted_channel_error'), 'error') return false end
if Config.RestrictedChannels[channel] ~= nil then
if not Config.RestrictedChannels[channel][PlayerData.job.name] and PlayerData.job.onduty then
Copy link

@Qwerty1Verified Qwerty1Verified Nov 12, 2024

Choose a reason for hiding this comment

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

Line 34 opens up a vulnerability for connecting to restricted channels because the logic is looking for whether the players job is not in the access list, and if they're on duty.

This means that if the players job is not in the access list, the first fail condition becomes true, but if they're off duty then the second fail condition is false which would allow them to connect to the radio because they don't meet the fail conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!! I used the same function it was before without questioning if it was right. I've just uploaded a fix.

Choose a reason for hiding this comment

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

Thank you for the changes. I think those conditions can be simplified though, more closer to the original code by just changing the comparison from and to or, and flipping the second condition.

I believe this would suffice and offer the same functionality:

        if not Config.RestrictedChannels[channel][PlayerData.job.name] or not PlayerData.job.onduty then

It reads as, if job is not whitelisted for the channel, it's a true fail condition, or if not on duty, it's a true fail condition.
This would mean if their job is not on the whitelist, it won't check the second comparison, and if their job is on the whitelist, it'll check the second comparison for whether they're on duty.

client.lua Outdated
if channel > Config.MaxFrequency or channel <= 0 then QBCore.Functions.Notify(Lang:t('restricted_channel_error'), 'error') return false end
if Config.RestrictedChannels[channel] ~= nil then
local isJobWhitelisted = Config.RestrictedChannels[channel][PlayerData.job.name]
if (not isJobWhitelisted) or (isJobWhitelisted and not PlayerData.job.onduty) then
Copy link

@Qwerty1Verified Qwerty1Verified Nov 12, 2024

Choose a reason for hiding this comment

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

Refer to my comment on the previous thread about changing these conditions on now line 35.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're tottally correct, what an stupid condition I did jajaja, being corrected right now

Choose a reason for hiding this comment

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

No worries lol, easily done. Thank you for your changes once again.

@mbiddle
Copy link
Contributor

mbiddle commented Nov 12, 2024

@Cocodrulo it looks like PR 52 was merged, so you may want to remove the duplicated phrase in this language as well.

@Cocodrulo
Copy link
Contributor Author

Done!!

@GhzGarage GhzGarage merged commit 94de37e into qbcore-framework:main Nov 12, 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.

5 participants