fix(database): prevent dupe, and add new placeholders #47
Conversation
… might need more looking at when we delete), change %pokemon% and %item% to %pokemonItem% in locale cus they use the same locale option.
danorris709
changed the title
| GTSAttribute attribute = ((ForgeEnvyPlayer) envyPlayer).getAttributeNow(GTSAttribute.class); | ||
| GTSAttribute attribute = envyPlayer.getAttributeNow(GTSAttribute.class); | ||
| attribute.getOwnedTrades().remove(this); | ||
| UtilConcurrency.runAsync(this::delete); |
There was a problem hiding this comment.
This shouldn't be here. We should only delete the trade once we are certain that the entire stack has been added to their inventory
There was a problem hiding this comment.
I think if the trade is removed from getOwnedTrades the db should reflect the same changes no?
There was a problem hiding this comment.
Also this is the pokemon trade, there is no stack.
There was a problem hiding this comment.
Yeah I didn't look at the class name there. However, I think this is still applicable as what if their PC is full?
There was a problem hiding this comment.
I think the current code actually doesn't handle that at all?
| TimeUnit.SECONDS.toMillis(EnvyGTSForge.getConfig().getMinTradeDuration()) | ||
| )) | ||
| .replace("%pokemon%", pokemon.getDisplayName().getString()))) | ||
| .replace("%pokemonItem%", pokemon.getDisplayName().getString()))) |
There was a problem hiding this comment.
Both types of DurationUIs use the same locale string, so I changed these two (this one and the question below) to be the same replacement string. Maybe I should change it to be something like "tradableName" to be a little more generic...
There was a problem hiding this comment.
Ah I see.
Yeah perhaps an underscore inbetween the words too to match the format of the other placeholders?
| TimeUnit.SECONDS.toMillis(EnvyGTSForge.getConfig().getMinTradeDuration()) | ||
| )) | ||
| .replace("%item%", itemInHand.getHoverName().getString()))) | ||
| .replace("%pokemonItem%", itemInHand.getHoverName().getString()))) |
There was a problem hiding this comment.
Same question here, regarding what was the reasoning for changing this?
|
|
||
| group = 'com.envyful.gts' | ||
| version = '5.1.0' | ||
| version = '5.2.0' |
There was a problem hiding this comment.
Not important for now but in the future you shouldn't include these changes in a PR
| } else { | ||
| returnGui.accept(player); | ||
| } | ||
| UtilConcurrency.runAsync(this::delete); |
There was a problem hiding this comment.
Delete call should not be here. The stack was not fully returned to the player's inventory
There was a problem hiding this comment.
It saves immediately afterwards. This deletion is for when the item partially got added, but some still couldn't be, which causes the item's count to be different, so we have to reflect that in the DB
There was a problem hiding this comment.
Yeah so we shouldn't be deleting it, we should just be updating the value in the database which is what the save call should be doing
There was a problem hiding this comment.
ok, this might just be me being SQL stupid, but does INSERT INTO update existing values? I thought it just creates a new one.
There was a problem hiding this comment.
Yes you are correct it is not updating. Perhaps this is the root cause of the issue? I have been assuming the save call was an INSERT INTO .... ON DUPLICATE KEY UPDATE etc etc
There was a problem hiding this comment.
possibly the cause of the issue. hmm.
| attribute.setCurrentMinPrice(0); | ||
| attribute.setCurrentPrice(0); | ||
| attribute.setSelectedSlot(-1); | ||
| PlatformProxy.runSync(player.getParent()::closeContainer); |
There was a problem hiding this comment.
Why did you add this here? Clicking the button should already close the UI
There was a problem hiding this comment.
Oh, that was me trying to prevent the error that pops up with dialogues (which didn't work), I guess I didn't notice and committed it too.
| } | ||
|
|
||
| UtilPlayer.runCommand(player.getParent(), "gts sell " + itemInHand.getCount() + " " + time + " " + submitted.getInput()); | ||
| PlatformProxy.runSync(player.getParent()::closeContainer); |
There was a problem hiding this comment.
Same again here, clicking the button should be closing the UI so why is this here?
2 participants
API Update, a few DB-related fixes, change %pokemon% and %item% to %pokemonItem% in locale cus they use the same locale option.