Habitica: space missing in Gem gift message

[Alys]

When gems are gifted to a player, the player receives a message saying "[person's name] has sent you X gems".

In languages other than the default en, there's no space between the number of gems and the translated word "gems". This is because the en version of the privateMessageGiftIntro string contains a space at the end of the string:
"Hello <%= receiverName %>, <%= senderName %> has sent you ",
but translators don't notice the space or don't realise it's important. It's not reasonable to expect them to notice it, so we need to fix this in code.

Relevant code:
https://github.com/HabitRPG/habitica/blob/0817cf96e1018a3858115b66ade304de600837e7/website/server/controllers/api-v3/members.js#L495-L499
Also two places in test/api/v3/integration/members/POST-transfer_gems.test.js

I'm not sure why privateMessageGiftIntro and privateMessageGiftGemsMessage are separate strings. They're only ever used together. Merging them into one string with three variables in it would fix this problem and likely would make the translations better for languages with different word orders than English.

[vincecampanale]

Hello Alys! I have not contributed to Habitica before. This seems like a manageable task for a newbie. Mind if I give it a shot?

[Alys]

@vincecampanale Please do! Thank you!

[vincecampanale]

Okay, I have made the necessary changes. Unfortunately merging privateMessageGiftIntro and privateMessageGiftGemsMessage means that the translations will have to be redone, however it is definitely the best solution. If the translations cannot be redone, I could keep privateMessageGiftIntro and just add a space to the beginning of privateMessageGiftGemsMessage, but 1) that's not really fixing the problem and 2) the space would have to be removed from the default en translation.

So how do I submit these changes for review?

[Alys]

In my opinion, it's fine to redo the translations for this. It will result in an improvement overall. Translators are often looking for extra work anyway :)

You can commit them locally, push them to your fork of the repository in GitHub, then from there do a pull request.

[vincecampanale]

@Alys Sounds good :) Just made the Pull Request!

[khipkin]

I'm concerned that privateMessageGiftIntro may be intended to be combined with privateMessageGiftGemsMessage OR privateMessageGiftSubscriptionMessage:

"privateMessageGiftIntro": "Hello <%= receiverName %>, <%= senderName %> has sent you ",
"privateMessageGiftGemsMessage": "<%= gemAmount %> gems!",
"privateMessageGiftSubscriptionMessage": "<%= numberOfMonths %> months of subscription! ",

[vincecampanale]

Ah you're right @khipkin. Good observation. @Alys is there a spacing error when gifting months of subscription as well?

I can either recreate privateMessageGiftIntro and concatenate privateMessageGiftGemsMessage with a space when constructing message (then do the same for privateMessageGiftSubscriptionMessage). Or, I can just apply the consolidation I used for privateMessageGiftGemsMessage in the above commit to the privateMessageGiftSubscriptionMessage. What do you guys think?

[lynxlynxlynx]

Can this wait until pr 8173 is merged? We'll have conflicts otherwise.

[vincecampanale]

@lynxlynxlynx Sounds good, let me know when it's ready.

[vincecampanale]

@Alys Now that PR #8173 is merged, how do I sync my fork and update my local version so I can try to tackle this problem again?

[lynxlynxlynx]

If you're not familiar with git, it will be easier if you ignore your previous commits and start fresh. Assuming you're on the master branch and you named the official repository (remote) "upstream":
git branch old8283 # save previous work
git reset --hard HEAD~2 # delete last two commits
git fetch upstream && git rebase upstream/develop

Then just do what you did before. You can look at your previous work on the PR via browser or by looking at the old8283 branch. Trying to merge it in would result in conflicts and that's not as easy to work through.

[vincecampanale]

Okay, I made the change that should fix the issue without breaking anything that depends on privateMessageGiftIntro.
What's the best way to test this change? Or should I just submit it for review since it is relatively small?

[vincecampanale]

Anybody know how to resolve this?

[TheHollidayInn]

Do git fetch instead. Then get rebase [branch]

[lynxlynxlynx]

And you'll have to force push to your fork (push -f).

[vincecampanale]

So the code is functional, as shown in the expected vs. actual output of the transfer gems test. (Expected lacks the space, actual has correct spacing -- so the test fails, but the output is what we want.) I'm having trouble with figuring out how to get the build to pass all of the tests. I could rewrite the tests and have it pass that way for the second of the build tasks, but the first one just says the "eslint" file isn't working. Any tips?

[lynxlynxlynx]

98:77 error Block must not be padded by blank lines padded-blocks

You added blank lines.

503:9 error Expected property shorthand babel/object-shorthand

JS ES2016 allows for specifying a key:value pair as just one of them, when they are identical. So don't construct anything like { a: a }

503:29 error Missing trailing comma comma-dangle

Another style thing, add a comma to the last element in the array/list/object. You'd think it's a syntax error, but no.

[vincecampanale]

All the tests passed! Thank you so much @lynxlynxlynx - that advice was extremely helpful for this PR _and_ taught me some new things.

[vincecampanale]

Okay pull request #8294 submitted with all the necessary changes!

[librarianmage]

This appears to have been fixed in #8463