Fundamentally speaking, aside from your bug, a lot of your code is pretty flawed and can be cleaned up. Some refactoring on your behalf not only helps to develop good practice but you could also end up fixing the issue in your code. That is, if the problem is caused by the provided segment.
Tidbits that would be helpful to know for improving your code.
-
Since you call UnlockTier multiple times for some products, consider adding an increment parameter determining how much should be added and go from there. There are several benefits to doing this.
-
Given the context of your function, you would best have an incremental parameter so it’s clear what the function can be used for and it allows you flexibility and customisation of increment amounts.
-
You do not need to establish a loop to use this function. Don’t use loops where they aren’t necessary. An increment parameter will skip the need for a loop when you can just add the value directly.
-
Given that you call FireClient every time UnlockTier is called, you can end up sending n amount of requests where n represents the second expression of your numeric for loop. This is bad for networking as you’re generating traffic unnecessarily. Use FireClient only after all your operations are done.
-
Don’t use WaitForChild unless your script is uncertain that EHpassInfo will exist at the time of the UnlockTier function being called. This is especially the case because you’d be using WaitForChild every time UnlockTier is called. In your current scenario, this is bad for performance.
-
You best implement a purchase history system that logs unique purchases as granted or unprocessed. ProcessReceipt is called multiple times until Enum.ProductPurchaseDecision is granted. Users could end up getting more tier unlocks than what they paid for.
-
Use GetService for the sake of consistency and canonical correctness. I assume you use GetService for MarketplaceService, you should do the same for the Players service.
-
GetPlayers is the canonical function for getting the list of players in a game. Don’t use GetChildren, that’s for other instances. Clarity in code is quite important for yourself and potential future collaborators.
-
Don’t use pairs to iterate through contiguous arrays, use ipairs.
-
Don’t use a for loop here at all. The engine provides a function that does the for loop for you internally so you can have less mess on your screen. The function is GetPlayerByUserId. Not only does using this function make your code look cleaner but it’s easier to work with for certain tasks. For example, not granting a purchase if the player doesn’t exist (which you currently don’t account for).
-
Consider using a dictionary where the key is a product’s id and the value is how many tiers should be added. You will perform a lookup on this dictionary using the ProductId in the receipt as a key and the value, if non-nil, as the increment argument for the new UnlockTier function. It’s much cleaner than constant if statements and allows you to scale better.
There’s one tidbit amongst this that needs to be highlighted outside of the block. It’s not something you can ignore and that’s why it needs to be outside of the optional fold. The following is not optional and your code must immediately be fixed to include it.
YOU MUST RETURN A PRODUCTPURCHASEDECISION ENUM.
Make sure to read the Developer Products article on the Developer Hub for more information on products and working with them. If you do not handle these products properly, especially in regards to returning the Enum, you will not receive any income from your passes and effectively be handing out free rewards at no charge. Do it wrongly and your game, possibly your account as well depending on the circumstances, may be suspended temporarily or permanently.
In order to save you on the moderation side, good coding practices side and all, I would highly recommend taking this advice into consideration going forward and refurbish your code accordingly. It’s more about just the bug here (which I still can’t discern a cause at this point), but it extends towards coding practices and moderation too.
I think it’s a good save that you posted this as a result of a bug because it extends to a much higher level of problems to handle.
If you’d like some code to get kickstarted, I can supply that for you since it just builds upon what you have and takes my earlier advice into consideration. Reading the tidbits would help you to understand what’s going on in my example better.
local EHTierProducts = {
[963255976] = 1,
[963256060] = 10,
[963256307] = 20,
[963301536] = 1,
[963301579] = 5,
[963301633] = 10,
[963301672] = 25,
}
local function UnlockTier(Player, Tiers)
-- If no Tiers argument is passed, use a default increment of 1
local Tiers = Tiers or 1
local EHpassTier = Player.EHpassInfo.CurrentTier
EHpassTier.Value = EHpassTier.Value + Tiers
UnlockTierRemote:FireClient(Player, EHpassTier.Value)
end
MarketPlaceService.ProcessReceipt = function(ReceiptInfo)
-- All it takes is one function. Look at how clean the rest becomes.
local player = game:GetService("Players"):GetPlayerByUserId(ReceiptInfo.PlayerId)
if not player then
-- Player is not in game: queue processing internally for later
return Enum.ProductPurchaseDecision.NotProcessedYet
end
-- Assume the server will always know an EHpass exists in the player
local EHpass = Player.EHpassInfo
-- Attempt to determine if the pass is valid
local GivenTiers = EHTierProducts[Receipt.ProductId]
if GivenTiers then
EHpass.IsUnlocked.Value = true
UnlockTier(player, GivenTiers)
end
-- Assume this part of the code is successfully reached with no errors,
-- purchase queues or anything. Tell Roblox the purchase was a success.
return Enum.ProductPurchaseDecision.Granted
end