HabitRPG / habitica

A habit tracker app which treats your goals like a Role Playing Game.
https://habitica.com
Other
12.04k stars 4.1k forks source link

"Invited a Friend" achievement is not awarded; party invitation is not sent (Social Signups only) #10786

Open Alys opened 6 years ago

Alys commented 6 years ago

This bug has been happening for ages (maybe since the new website) but I've just realised we don't have an issue for it, unless I can't find it. shrug

When you invite someone to Habitica and they register an account from the link in the email they received, you are supposed to get the invitedFriend achievement. In some or all cases (I'm not sure which), the achievement is not given.

Also they are supposed to receive an invitation to your party but they don't.

SabreCat commented 6 years ago

I think this is happening because the link that people get on invite just goes to the front page. It doesn't hook into anything.

Alys commented 6 years ago

There's some additional details found by cTheDragons (on the delta test site but I expect it's the same for the production site):

"When inviting a new player to Habitica via their email. When the account signs up with that email account, the player does not automatically join the party. If you invite them again with their email account it does appear in the accept/reject which you can then accept."

Alys commented 5 years ago

We had a report of this today so I'm copying it here as evidence that this bug still happens. We actually see a lot of reports about it but this one more clearly shows both impacts of the bug. It's from @schmyddy (9a3f4c14-3527-48c9-8d49-231cc353b7ed):

"Hey there. I face 2 problems right now: [I use the german version, so forgive me if i translate something wrong]

  1. I have invited someone to my party to get the Inviting-a-friend achievement through entering her e-mail and she clicked the link and registered but doesn't have any notification of an invitation.
  2. I tried to invite her now through her user name, but when i click "send invitation" it says, that this user doesn't exist... But she send me a picture of her profile and she already used both the app and the browser version of Habitica, so she should definitely exist. Thank you for your effort in advance and thank you for taking care of the community. :)"
SabreCat commented 5 years ago

Upgrading to Important as it's a constant drain for user support.

nikonhub commented 5 years ago

I tried to invite an email and I think the problem should come from mandillapp configuration.

Because this link : https://mandrillapp.com/track/click/30295795/habitica.com?p=eyJzIjoiY2dDMGcxbnRmdlBWNGlLRGprdDFxZ0hfYTh3IiwidiI6MSwicCI6IntcInVcIjozMDI5NTc5NSxcInZcIjoxLFwidXJsXCI6XCJodHRwczpcXFwvXFxcL2hhYml0aWNhLmNvbVxcXC9yZWdpc3RlclwiLFwiaWRcIjpcIjVjNmQ5YTBkNmEyZjRlZjI4NjUwOTgwODYzYTEzOWU0XCIsXCJ1cmxfaWRzXCI6W1wiYjg0ODcxNjZiYzQ2NjlmZDIxY2EyZmNiZTIzYjM1ODE5MWFkMGM5N1wiXX0ifQ

redirect with 302 http code to : https://habitica.com/register

Secondly even if p param is passed to /register, this component does not handle it but /static/home does.

SabreCat commented 5 years ago

Attempted a simple fix in https://github.com/HabitRPG/habitica/commit/a32622c81fd231b9da04ecf2767e4c20f16ad3e7, we'll see how it goes!

SabreCat commented 5 years ago

No luck yet. It's still rerouting to naked /register instead of /static/home with the param.

paglias commented 5 years ago

Hmm there's probably a condition in the client side routing that triggers the redirect to the register page instead of the home. But if you manually type (in an incognito browser window) https://habitica.com/static/home?groupInvite=aaaa you don't get redirected so it might be a mandrill problem

paglias commented 5 years ago

Just tested the fix and it works πŸŽ‰

Alys commented 4 years ago

I'm reopening this because the bug still exists if the invited user signs up using the Google sign-in method (even with a Google account that uses the same email address as the inviter entered into the invitation form). The user does not get an invitation to the party and the inviter does not get the invitedFriend achievement.

Steps followed in my test:

  1. Check that my lady.alys@gmail.com address is not used in any account (NB for anyone concerned, my gmail address is already public knowledge).
  2. Used my @alys account (d904bd62-da08-416b-a816-ba797c9ee265) to send a party invitation to lady.alys@gmail.com (party ID ae9bf3a2-8050-4253-85ba-14eab6b19e72 - that's a real party not a test one so other admins shouldn't try to use that party ID for anything other than searching logs).
  3. Received the invitation email in my gmail account.
  4. Opened the link in a Firefox Container that did not already have a Habitica session logged in.
  5. Used the Google sign-in button to create an account.
  6. Confirmed that the new account did not have a party invitation.
  7. Confirmed that my @alys account did not have the achievement.

I assume but have not tested that the process would also fail if the Facebook button was used. It would be advisable for that to be tested though if someone wants to do that.

The bug does NOT occur if the invitee creates an account using "local" login (Username + email address + password). In my test of that situation, the invitee does get an invitation to the party and the inviter does get the achievement.

paglias commented 4 years ago

Hmm I can see why this happens, we'll have to store the invite in localstorage

benkelaar commented 4 years ago

@Alys I can take a look at this one next.

@paglias Isn't it an option to have the groupInvite query parameter also be part of the callback url that's send to the social logins? (Assuming that's how they work)

Alys commented 4 years ago

@benkelaar I've set this as in progress. Thanks!

benkelaar commented 4 years ago

I'm making good progress on this issue, have a clear idea of how to fix it but there's some work in making it testable, but also good progress there.

@Alys In order to get the most out of local testing, do you know if I can find the e-mail templates somewhere for email types invited-guild and invited-party?

I've looked at the repository of the mail server but perhaps the templates themselves are only available with a mailchimp or mandrill account? Or am I missing something?

~The only thing I really need to know is how exactly the invite link is created, I can kinda infer from the arguments and the parameter handling, but seeing the actual source would be better.~

Edit: Found the link creation here: https://github.com/HabitRPG/habitica/blob/develop/website/server/libs/invites/index.js#L179 so I think my question is a moot point by now (apologies)

paglias commented 4 years ago

I've looked at the repository of the mail server but perhaps the templates themselves are only available with a mailchimp or mandrill account? Or am I missing something?

Yes the templates are only available in the mandrill / mailchimp templates but all variables can be found in the code here or in https://github.com/HabitRPG/habitrpg-email-server

@paglias Isn't it an option to have the groupInvite query parameter also be part of the callback url that's send to the social logins? (Assuming that's how they work)

Hmm I don't remember exactly the cause here but I think the problem is that we redirect the users (for Google/FB in a popup) and when they complete the login they get redirected back to Habitica but the URL they're redirected to isn't really customizable with extra parameters.

An idea, that would make it work on mobile as well, would be: When the invite is sent, save it in a database collection -> when the user sign up with that email the inviters get the achievement. Would make it possible to remove the special URL. Issue: would only work if the sign up email matches the invitation one

opinions?

benkelaar commented 4 years ago

That does limit users to use the same e-mail they were invited on. I can imagine inviting a colleague on their work e-mail but them wanting to sign up on their private e-mail.

I think I can fix the current setup though. Right now only the register method is reading the query parameter. In principle the socialAuth method can just read that same parameter and send it on to the dispatched auth:socialAuth action, which can then post it to the backend which can then simply call _handleGroupInvitation from loginSocial in the same way that registerLocal is doing it. Pretty sure that should fix this. (Only Apple I think would need some special care).

Edit: The reason I haven't finished it yet is because I've built a unit test for the home.vue component. However there's an issue in the test isolation of the different Vue.js component unit tests. Even though I set up a local Vue for the home.vue unit test, asynchronous events get picked up by other tests which then do not have registered handlers for them meaning they randomly start throwing errors. I've tracked down several theories of why this would be happening, but haven't found the reason yet.

paglias commented 4 years ago

I think I can fix the current setup though. Right now only the register method is reading the query parameter. In principle the socialAuth method can just read that same parameter and send it on to the dispatched auth:socialAuth action, which can then post it to the backend which can then simply call _handleGroupInvitation from loginSocial in the same way that registerLocal is doing it. Pretty sure that should fix this. (Only Apple I think would need some special care).

πŸ‘ i thought that the call to the sign up route happened in the popup used to redirect to facebook and google but it doesn't look like it.

For Apple yeah it's a bit more difficult since there's no client side handling but you get a redirect from the main windown and not from a popup (they do not support client side oauth unfortunately)

benkelaar commented 4 years ago

Yeah, so for apple we'd probably need to add the same query parameter to the redirect url that we're sending to apple so that the invite can be handled then. However that would probably require a custom landing page to handle that query parameter.

Ideally you'd abstract the handling of this parameter away into something like a request filter. Do you know if Vue.JS supports a concept akin to request filters?

However, looking at the amount of change that's going on in this code in #12141 I think I may hold off on these changes until that one's been merged.

I'll continue my investigation into the Vue.js component unit tests, do you have any tips there for why the triggered events might be breaking isolation?

paglias commented 4 years ago

Yeah, so for apple we'd probably need to add the same query parameter to the redirect url that we're sending to apple so that the invite can be handled then. However that would probably require a custom landing page to handle that query parameter.

I think the problem is that any query param sent to Apple would get lost when they redirect back to us and the redirect url can’t be passed customizable parameters

benkelaar commented 4 years ago

I think the problem is that any query param sent to Apple would get lost when they redirect back to us and the redirect url can’t be passed customizable parameters

Can't you add a query parameter to the redirectUrl here?https://github.com/HabitRPG/habitica/blob/develop/website/client/src/libs/auth.js#L26 ? Or does apple strip query parameters?

paglias commented 4 years ago

No, basically you have to register, on the Apple Sign In platform, a list of allowed redirect url, if you pass a different one the request will not be considered valid. And the query parameters are considered part of the URL so ${window.location.protocol}//${window.location.host}/api/v4/user/auth/apple?inviteCode=code1 is different than ${window.location.protocol}//${window.location.host}/api/v4/user/auth/apple?inviteCode=code2 and to be valid all possible invite codes would have to be whitelisted

benkelaar commented 4 years ago

Ah right. That makes sense.

Obvious alternatives are allowing people that signup via apple to specify which user invited them or just showing a message along the lines of ("Due to technical limitations of the apple authentication integration invites don't work please ask your inviter to invite you by username. "). Theoretically you could also not show the apple auth option when a user was invited, but that probably annoys iPhone users πŸ˜…

A quick Google also suggested you could use Apple.JS instead of the auth REST API. Haven't checked any details yet but perhaps that might be an option.

paglias commented 4 years ago

A quick Google also suggested you could use Apple.JS instead of the auth REST API. Haven't checked any details yet but perhaps that might be an option.

I don't remember why we didn't use it but @phillipthelen might know

As for the issue itself another solution, since we're only targeting web right now, would be to set a cookie with the invite data when the invitation link is opened, and on signup award the achievement based on the cookie data. It would support all sign up mechanisms including Apple. We could leave the query parameter support for mobile

benkelaar commented 4 years ago

That sound like a pretty clean solution.

Is there a possibly bad interaction with cookie opt-out? (Not sure if habitica offers that but it's probably acceptable if this functionality doesn't work when someone expressly opts out of all cookies)

paglias commented 4 years ago

Is there a possibly bad interaction with cookie opt-out? (Not sure if habitica offers that but it's probably acceptable if this functionality doesn't work when someone expressly opts out of all cookies)

Shouldn't be an issue, if first party cookies are disabled at the browser level it won't work but not much we can do

Alys commented 4 years ago

[This is for my reference only and can be ignored by everyone else. I've marked this as notify helpers to remind me to document the conditions under which the achievement still fails to be given when a fix for this has been merged; e.g., if the new player signs up with an Apple ID. We'll need to know that for user support.]

shanaqui commented 4 years ago

@benkelaar Just checking in whether you're still working on this one! If not I'll mark it as "help wanted" again. :)

benkelaar commented 4 years ago

@shanaqui I am holding off work on this issue until this merge request #12141 is merged since it heavily touches all the same related code.

shanaqui commented 3 years ago

Hi @benkelaar! Just checking in about this one since that PR has been closed rather than merged; are you still hoping to work on this, or shall I open it up for other contributors to try?

davishuang9 commented 3 years ago

@shanaqui or others: I'm willing to give it a try! but this'll be my first time diving into the habitica codebase and being part of the habitica process. what's the turn around time expected for issues like this?

Alys commented 3 years ago

@davishuang9 For an issue like this that's been happening for at least three years, we wouldn't expect a rapid fix. :) When an issue is claimed by someone, we check back every so often to see how it's going, with the frequency depending to some extent on the urgency of the issue. See also Using Your Local Install to Modify Habitica%27s Website and API for information about the PR process.

I'm marking this as in progress for you. Thank you for volunteering! Post here if you have questions or run into trouble.

Note that the fix may be

beffymaroo commented 3 years ago

@davishuang9 Are you still working on this issue? Please let us know if so. If not, I will mark it as Help Wanted again. Thank you!

davishuang9 commented 3 years ago

@beffymaroo hi sorry not going to be able to get to this issue in the time being.. work got busy! i appreciate the opp. and will look for a better time to contribute

megan-starr9 commented 1 year ago

@SabreCat I have been meaning to set up the habitica repo on my computer, this seems like a good issue to tackle. I don't mind looking into the cookie solution πŸ‘

CuriousMagpie commented 1 year ago

@megan-starr9 Thanks for being willing to take this on!

TicoM1 commented 1 year ago

Stumbled upon this after missing out on the achievment. Just to be sure: @benkelaar pretty much solved the issue, held back to wait for another request and three years later it's still not allowed to use social logins? Is it possible to adapt the achievment's description to let the user know?

GitAritron commented 3 months ago

Seems like the place to get the invite link itself doesn't exist on the android app.