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: Antidotes to Avatar Transformation Items should be added to Rewards by API #11353

Merged
merged 9 commits into from Oct 6, 2019

Conversation

Xaz16
Copy link
Contributor

@Xaz16 Xaz16 commented Sep 7, 2019

Fixes #11289

Changes

  • Move logic of choosing proper debuf potion from vue component to website commons
  • introduce new function to get debuffSpellItems
  • introduce set debuff spell item function to understand what should we do with user which have some of buffs about transformation, delete or set antidote to pinned items in rewards
  • introduce new type for debuffPotion to recognise and properly use it on client

At all I can say that it is minimal solution to satisfy the issue report, in fact I tried to move anitdotes to user inventory, but I face some extra work to make it work as "self" spell. So for now -- we retrieve it in pinned items of user data which we receive from data and I'm not significantly edit website code to work with such item.

Here we have issue -- if I unpin the item from rewards sections I can't return it back due to we don't have it in inventory at all, if we check transformation items it is in inventory and I think antidotes should be too. To forbid unpin for such debuffPotion type items we can broke path in content of item, but it is not a clear solution for it...

I open for discuss to understand in which way we should store it and how it should work, feel free to participate :)


UUID: d94eca66-c6de-423c-a864-9cb103b9eb77

- Move logic of choosing proper debuf potion from vue component to website commons
- introduce new function to get debuffSpellItems
@Xaz16 Xaz16 changed the title Fix: when buying new equipment, item does not disappear from Market or Seasonal Shop (not ready solution) Fix: Antidotes to Avatar Transformation Items should be added to Rewards by API (not ready solution) Sep 7, 2019
@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 7, 2019

@Alys I have made preparation for getting correct antidote (moved it to common) function to use it then on server. Can you please review and answer if I'm going in right way? I'm really new in this project, so will be happy about your feedback :)

@Alys
Copy link
Contributor

Alys commented Sep 8, 2019

@Xaz16 That's something that Habitica's staff should comment on so I'll leave it for them to answer you but thank you for checking with us!

@Xaz16 Xaz16 changed the title Fix: Antidotes to Avatar Transformation Items should be added to Rewards by API (not ready solution) Fix: Antidotes to Avatar Transformation Items should be added to Rewards by API Sep 9, 2019
@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 9, 2019

@Alys I finished work on this one, could you please put "ready for review" label? Thank you

@paglias
Copy link
Contributor

paglias commented Sep 9, 2019

@negue might have some ideas about the pinned items issue

@paglias
Copy link
Contributor

paglias commented Sep 9, 2019

@phillipthelen is this going to cause any issue with the mobile apps? (A new type of items added)

Copy link
Contributor

@paglias paglias left a comment

Choose a reason for hiding this comment

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

left some comments! Note that I haven't live-tested it

website/common/script/index.js Outdated Show resolved Hide resolved
website/common/script/libs/getItemInfo.js Show resolved Hide resolved
website/common/script/libs/setDebuffPotionItem.js Outdated Show resolved Hide resolved
@negue
Copy link
Member

negue commented Sep 9, 2019

  1. On the pinning of debuffPotions we could
    a) not let those potions be unpinned
    or
    b) add a pin on the items page for these potions

  2. Also the pinned potions seem to have an image at the Rewards-Column.

  3. If you have one debuff and cast another one, then two debuff-potions are pinned

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 10, 2019

Hi folks!
@negue

  1. Agree with b) but it will be only market one, due to debuff potions is immediateUse items... Or is it not a good idea to extend functionality of market, because on web app we don't have such functionality for immediateUse items. I think the a) would be better but still it means that android/ ios applications should implement "not unpinned items" functionality too. @paglias what do you think about it?
  2. Sure,
    image
  3. Agree, will cover my function setdebufPotionItems with appropriate tests. But in fact due to web app logic we can't have more than 1 debuff on user https://github.com/HabitRPG/habitica/blob/develop/website/common/script/content/spells.js#L356 (as example seafoam in cast set to false all other debufs)

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 10, 2019

@paglias what do you think about @negue summary and my questions about it above?
Could you please set labels for this PR as work or something about it?

@phillipthelen
Copy link
Member

@paglias Adding new items should not cause an issue with the apps. But it kinda depends on how exactly these are "purchased". Which API call would this use?

@paglias
Copy link
Contributor

paglias commented Sep 10, 2019

@phillipthelen they would come through the content api, as far as purchasing they are added to your rewards list

@paglias
Copy link
Contributor

paglias commented Sep 10, 2019

Hmm @Xaz16, yesterday I had missed the fact that these are only implemented as pinned items, would it be possible to have them as normal rewards without having them appear in the market?

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 10, 2019

@paglias can you elaborate please? Should it work as an heal potion (as it works now)? It mainly looks like as you wish, but it is strange thing -- if I will unpin heal potion, does it mean that I lose ability to buy them?

Of course I will do as you said, but I need a comprehensive requirement.

@paglias
Copy link
Contributor

paglias commented Sep 10, 2019

@Xaz16 sorry I wasn't very clear but to be honest I think this is a question that @Alys is more suited to answer than me.

It should work like an heal potion but that one is always available in your rewards list, it's not a pinned item. I think that this should work like that so it should also appear in the market (only if you can buy it)

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 10, 2019

@paglias ok, I tested that we can't unpin health potion and will do the same thing with debuff potions. Thank you :)

@Alys should debuff potions be available in market if user have any transform buff as @paglias said?

- Create test case for get and set DebuffPotionItems functions
- Fix setDebuffPotionItems function to not create duplicated debuff items
- Make debuff potion type of items unpinnable
- Move list of debuffs to constant to reuse it in tests and functions
@Alys
Copy link
Contributor

Alys commented Sep 10, 2019

I agree that having the the debuff item work in Rewards like the Health potion is good - i.e., you shouldn't be allowed to unpin it. (We might get a couple of complaints about that but we can easily explain that it's much better to avoid accidental unpinning.)

I don't have a strong opinion about whether the debuff item should be in the Market or not. On the one hand there's no real need for it there but on the other hand it wouldn't hurt if it was there. From memory, it wasn't in the Market on the original website.
For code neatness, I suggest going with whatever the Market's default state is - if the debuff item appears automatically in an appropriate section in the Market, leave it there; if it doesn't appear, then don't create code to add it.

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 11, 2019

@Alys thank you. As I understand my current code already satisfy all that you write. I just want to notice a message that we have when we try to unpin unpinable item --
"cannotUnpinArmoirPotion": "The Health Potion and Enchanted Armoire cannot be unpinned."
For some reason it is strictly bound to the especial items... What should we do in this case?

Could you set appropriate label please?

@Xaz16 Xaz16 requested a review from paglias September 11, 2019 05:42
@Alys
Copy link
Contributor

Alys commented Sep 11, 2019

This should do for all items that can't be unpinned:
"This item cannot be unpinned."

Thanks for asking!

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 11, 2019

This should do for all items that can't be unpinned:
"This item cannot be unpinned."

Thanks for asking!

Ok, but it is a task for linguists. I'm I right or should I change translation only for en and rename key of translation to appropriate one like "cannotUnpinItem"?

@Alys
Copy link
Contributor

Alys commented Sep 11, 2019

You can change just the en version - see the _README_FIRST.md file in the en directory for details.

It's probably a good idea to rename the key for clarity as you suggest. It's used in only two places so it won't be hard to update them both.

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 11, 2019

@Alys, ok got it, thank you :)

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 24, 2019

Hi @paglias, any plans for this PR? Should I extend or fix something?

@paglias
Copy link
Contributor

paglias commented Sep 24, 2019

@Xaz16 sorry for the delay, there are no problems with the PR but I've not been able to review it in depth last week, should get to it this one

@Xaz16
Copy link
Contributor Author

Xaz16 commented Sep 24, 2019

@paglias got it, thank you for attention :)

Copy link
Contributor

@paglias paglias left a comment

Choose a reason for hiding this comment

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

Left one comment! I'm going to put this on a testing website for a few days

website/common/script/content/spells.js Outdated Show resolved Hide resolved
@paglias
Copy link
Contributor

paglias commented Oct 6, 2019

@Xaz16 merged in develop, after some final testing this week it should go live around Friday. Thanks for the PR! Notated towards your 3rd tier

@paglias
Copy link
Contributor

paglias commented Oct 7, 2019

Looks like the mobile apps require an update to work with this PR so I'm going to revert it and merge it again once they're ready

@Xaz16
Copy link
Contributor Author

Xaz16 commented Oct 7, 2019

@paglias ok, got it. Thank you for info :)

@paglias
Copy link
Contributor

paglias commented Oct 7, 2019

Reopened at #11407

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.

Antidotes to Avatar Transformation Items should be added to Rewards by API
5 participants