habitica
habitica copied to clipboard
HabitRPG
No error message when hatching a pet fails because you already own it
citrusella posted in RaB: "While hatching pets for the testing I just did, I realized that messageAlreadyPet ("You already have that pet. Try hatching a different combination!") doesn't pop up when you try to hatch a pet you already have. Is that deliberate or a bug?"
I have that issue too, and I'm guessing "bug", because it means you're just getting silent failures when you try to hatch a pet from the inventory with a potion and egg that you've already used together.
@seunggyupark I've marked it as in progress for you
@vfpereira unfortunately an issue can only be claimed by one person
I found no issues with the logic or the test for this. Could it potentially be releasing a pet which sets its' value to 0, then trying to hatch it again? I'm wondering if this if statement:
if (user.items.pets[pet] && user.items.pets[pet] > 0) { throw new NotAuthorized(i18n.t('messageAlreadyPet', req.language)); }
Should be changed to just
if (user.items.pets[pet] > 0) { throw new NotAuthorized(i18n.t('messageAlreadyPet', req.language)); }
to address that scenario. I'm trying to get the local environment set up so I can do a more thorough investigation rather than what I've seen just by looking at the test and the code. I don't think its' a styling issue either, NotAuthorized extends CustomError like all the other error messages.
Would this be a vue issue? (I'm familiar with React).
@seunggyupark that could be it! As user.items.pets[pet] with a value of 0 would be considered a false-y value in javascript
@seunggyupark When a pet changes to a mount, the pet's value is set to -1, not to 0. The value is never 0, not even for a new pet or a re-hatched pet (both start at 5).
Maybe we want to consider changing the if statement to this?
... user.items.pets[pet] > -1) { // -1 = pet tamed but not re-hatched
That won't fix this issue of course but it will make the behaviour clearer.
If relevant (I'm not sure if I'm reading and understanding the conversation right): I should note that when I encountered this issue on my test account, it had not released the pet in question before--I don't have beast master, mount master, or triad bingo on that account.
(This is probably not relevant but there you go?)
Thanks for the comments. I'm trying to get the local environment set up but having some issues with memory when running npm run serve. @citrusella did it still add the pet even though you encountered the issue?
I already had the pet. That's the issue. It couldn't add the pet because I already had it, and it presented no "you already own this pet" error message like it used to--the hatch just silently doesn't happen, and the egg and potion stay in the inventory. I had been going down the list of pets of a certain color to hatch them all and see if I got an achievement someone was reporting a different possible issue about, and a few times I accidentally clicked a pet I'd already hatched when I was doing it and noticed the lack of any notification that I had done that kind of thing.
Maybe I'm confused about what's being asked?
Hi @seunggyupark! Just checking in -- are you still working on this issue?
Not actively due to holidays, I need to set up a Mac VM to install the project as it seems the Windows install has memory problems. If someone else wishes to pick it up that is fine by me, but if it's still not done I will get around to it after New Years.
No worries, it's not that we're in a rush! Just checking that the issue hasn't been abandoned. :)
@shanaqui Hi, sorry I've gotten quite busy and having too much trouble setting up a VM to get to this issue. Is someone else able to pick it up?
@seunggyupark Sorry I missed your message! Yes, I'll set this back to Help Wanted. :)
I don't think I'll have time to get to it any time soon, so opening it back up.
Based on the testing I did using the console, the hatch function in hatch.js isn't running at all if the user already has the pet.
I made the conditional statement (1>0) and it still wouldn't run. But when I hatched a "hatchable pet/egg" then my console message and the error message showed up (because I changed the conditional statement to see what would happen).
Now I need to find out where the eventHandler logic starts. It seems that the error logic (when a user cannot hatch a pet) needs to be moved out of hatch.js, but I'm not sure where it would go until I hunt down the beginning of this hatching process.
I may be sniffing up the wrong tree, but is it possible this is where the hatching gets its start? (It definitely contains logic (near the end of what I copied) that checks if a pet is hatchable and hatches it if you've clicked both an egg or a potion (in either order).)
https://github.com/HabitRPG/habitica/blob/8070486def93ba9f73e23f30f4ae2171dd89f82c/website/client/src/components/inventory/items/index.vue#L513-L596
Ooooh. Thanks @citrusella - I had looked at that file yesterday, but looking again I'm specifically noticing lines 553-559 and beyond , I'll look more into that...
Hmm...I think this is egg-actly what I'm looking for...🥚. Thank you! 🙏🏽
I'm not sure that the error thrown in hatch.js (lines 41-43) was ever actually working..the commit message said "wip: split shared ops" and when I edit index.vue (line 551, isHatchable()) and force the message to get called, I get an uncaught Promise error.
There also seems to be other unfinished code in index.vue in regards to making eggs draggable...
If you see other parts of the code that you'd like to finish up while you're working on this issue, go ahead and do so! Just be sure to document them clearly in your PR and check for any relevant issues that your fixes may resolve.
I'm not sure that the error thrown in
hatch.js(lines41-43) was ever actually working..the commit message said "wip: split shared ops" and when I editindex.vue(line551,isHatchable()) and force the message to get called, I get anuncaught Promiseerror.
For what it's worth, I'm fairly sure the notification/error worked at one point, as I was aware of it enough to note its absence and that's the whole reason I reported it originally. However (this is a hypothesis and not what definitely happened), it's possible that the last time it definitely worked was on the previous design of the website (which stopped getting used late September 2017), maybe, which might make sense? It used an entirely different set of files (Angular JS as opposed to the current Vue) that are not in the current version of the repository anymore and maybe the error played nicer with that code? Somehow?