search

hippware / rn-chat

MIT License
5 stars 0 forks source link

Onboarding 4: Invite Friends #3045

Closed thescurry closed 5 years ago

thescurry commented 6 years ago

Ingest of phone contact list and inviting friends is next.

Please see the following Zeplin mocks:

https://zpl.io/adPzXql

https://zpl.io/29PwK8p

https://zpl.io/VQ9QDL5

https://zpl.io/bWkNj91

https://zpl.io/2ZkDQD1

cc: @southerneer @aksonov @bengtan @irfirl

southerneer commented 6 years ago

Find Friends list sort order priority:

  1. following
  2. alpha by displayName (handle if in following list, first last otherwise)
southerneer commented 6 years ago

@bengtan please link any relevant backend tickets here and remove the Blocked label when they're ready.

bengtan commented 6 years ago

Server side tickets:

https://github.com/hippware/wocky/issues/1974 Research and/or implement SMS integration

New API to upload a list of contacts and send invites to them https://github.com/hippware/wocky/issues/1975

bengtan commented 6 years ago

If a user is already friends with any existing contacts, they will appear with "following" (this will probably NOT be the case for many users initially) If a user is NOT already following someone on the list, then "invite" appears

@thescurry:

Uh, there's a third case in between those two.

What if the contact is already on tinyrobot but not followed by the user? Still show 'invite' (by sms)?

thescurry commented 6 years ago

Hmmmm... @irfirl what do you think? I doubt it's something we run into today, but we should disucss how to handle the issue. Maybe we should have a section that shows contacts who are already on the platform that you can simply "follow"?

We should discuss "following" versus "being friends" while we are here in LA. Since the app is now very private, we may want to reconsider or at least discuss how users connect.

irfirl commented 6 years ago

@bengtan I updated the screens to include contacts that are already on tinyrobot: https://zpl.io/bWkNj91 and https://zpl.io/2ZkDQD1. A few changes to note, however, we're changing the terminology of "follow" to "connect" and then "following" to "friends" because it makes more sense now that all bots are private.

We should also update the "Updates" screen to reflect these changes: https://zpl.io/2plyQxM

southerneer commented 6 years ago

I don't understand the difference here between 'sent' and 'invited'. Or 'connect' and 'invite'

irfirl commented 6 years ago

Connect means they're already on the app and invite are for those that are not on the app. Sent means you've sent them a request, and invited means you've invited them to the app.

bengtan commented 6 years ago

then "following" to "friends" because

This is potentially changing existing usage and could cause confusion if we don't first clarify and communicate to everyone.

Currently, following and friends have two distinct meanings. 'Following' is in one direction, and friends is bidirectional, reciprocal following.

So ...

  1. This is changing terminology. Are you aware of this? Are you sure you want to do this?

  2. (Using old terminology) Following another user doesn't mean that they'll necessarily follow back. There is no (bidirectional) 'friend' relationship (yet) So, for example, they can't send a DM to someone they follow (unless they follow back).

cc @irfirl @thescurry

thescurry commented 6 years ago

@bengtan we have no active users currently and because of that, I'm not worried about confusing anyone with a change in wording. I think if we have a private'ish app, using the term "follow/following" doesn't necessarily re-enforce the idea of private or privacy. So our goal is to make the wording work for the type of app we've pivoted to. I think we'll be fine, but it could be confusing for us while we sort this wording change.

Happy to discuss this during the afternoon...

I other words, this app only works if you have friends to connect with and share your location. To that point, it no longer makes sense to follow or have followers. This is more or less cleanup/polish...

thescurry commented 5 years ago

Hey @bengtan let's add this to our discussion agenda tonight.

southerneer commented 5 years ago

From https://github.com/hippware/rn-chat/issues/3023#issuecomment-441772719...

https://zpl.io/bPdleyl is meant to replace https://zpl.io/V4JKLG4 (the existing Location permission warning screen)

southerneer commented 5 years ago

@toland @bernardd with userBulkLookup and friendBulkInvite this is unblocked on the backend, correct?

bernardd commented 5 years ago

If you're asking if it's ready, yes it is (at least in it's initial form - let me know if you need any changes).

southerneer commented 5 years ago

~@thescurry for the contacts permission description I've temporarily added...~

~"We use your contact list to find other tinyrobot users and allow you to invite friends who may enjoy the app."~

~Let me know if you want to replace that.~

Never mind, I forgot the wording was on the Zeplin already

southerneer commented 5 years ago

Once a user taps "invite" and the server acknowledges that it received the request to invite, the text is then updated to "invited" (this point might require some discussion).

Inviting users is a one-shot deal at the moment, correct? In other words, there's no UI/screen for a user to go back and check the status of their contact invites later. If that's true and we don't see a need for that in the near future, it's not a big deal to flip the text to "invited" because we don't have to worry about checking that value later (after a background/foreground/reinstall).

UPDATE: talked it over with Steve and Alan and my assumption above was correct.

bengtan commented 5 years ago

Question please (@thescurry @irfirl).

A phone contact may have more than one number. What is displayed in that case?

Let's say that the phone contacts include:

And on the server side, 555-BBB-0002, 555-CCC-0002 and 555-CCC-0003 are three distinct existing accounts.

(AAA, BBB, CCC are placeholders)

In the Friends List screen (ie. https://zpl.io/bWkNj91) ...

Alice

Alice has two phone numbers, both of which are not an existing account.

Does Alice get listed once or once for each phone number (ie. twice)? If twice, then how does the user know which listing is 555-AAA-0001 and which is 555-AAA-0002? I think the phone number would have to be displayed under the name so the user can disambiguate?

Display phone numbers in general

To extrapolate this further, I think it could be argued that for all listings which are not an existing account, the phone number should be displayed. This provides visual cues to the user.

For example, I might have a contact 'Next door neighbour' with a phone number for their home. It's not a mobile number. It's the phone number of the home and SMS cannot be sent to it. If the user can see the phone number (and realises it's not a mobile number), then they (hopefully) won't try to send an invite to it.

Bob

Bob has two phone numbers, one is an existing account, the other is not.

Is Bob listed as an account (ie. 'Connect' CTA) or a non-account (ie. 'Invite' CTA), or is Bob listed twice (once as an acount, once as a non-account)?

If listed twice, should there be some sort of visual cue so the user can see that the listings are related?

Sort order

AFAIK, the current requirements say that accounts are listed first, followed by non-accounts.

If Bob is listed twice, once as an account, and once as a non-account, then the two items may be quite far apart. Is this a problem?

Cathy

Similar to Bob, but with an added dilemma.

Cathy has phone numbers which correspond to TWO existing accounts. This should never happen, but the client needs to be able to handle it nonetheless. We can use the same answers (and rules) that apply to Bob. But do we want to treat this situation even more carefully/specially?

irfirl commented 5 years ago

I think if the user is already on the app, they don't need to be invited again through sms so we can just show the portrait with the 'connect' button to add them directly.

I updated https://zpl.io/bWkNj91 to display phone numbers on the second line for those that are in contacts but not on the app.

If a contact has more than one number, I think it should automatically pick the most likely one that they'll get the invite message. From looking at the list of labels, I think it should be in the following order:

  1. main
  2. mobile
  3. iPhone
  4. home
  5. work
  6. other excluded: home fax, work fax

If someone has a phone number connected to two accounts. I think we can just display both of the accounts (if its not much work).

thescurry commented 5 years ago

^^ @bengtan thoughts on this? Happy to discuss tonight via Slack.

bengtan commented 5 years ago

Quoting @irfirl:

If a contact has more than one number, I think it should automatically pick the most likely one that they'll get the invite message.

Keep in mind that labels are not always accurate. I don't know how accurate they are, on average. For some people, or parts of the population, it could be very inaccurate.

If you list all numbers instead of just the 'most likely' one, the user can choose the correct one. That's much more accurate ... but at the expense of the user having to do their own checking.

Hmmm ...

Or another alternative ... for a multiple-number contact, we could list them once, and then send SMS to all their phone numbers (if they are chosen).

Which makes me think of something else.

Some phone contacts might not have a mobile number ie. they are an email-only contact. These should be just omitted from being displayed, I think.

Quoting @thescurry:

thoughts on this? Happy to discuss tonight via Slack.

Should be fine. It's not a tricky topic, just details.

thescurry commented 5 years ago

Agree, let's omit email only contacts for now.

bengtan commented 5 years ago

Note:

The dynamic invite links which are triggered by the CTAs in this ticket are server generated and the server's responsibility.

As currently configured, the Testing, next and Staging servers don't actually send out SMSes. Instead, there are sent to a sandbox/black-hole which just ignores them (albeit logged to Loggly). Only Production/us1 sends out real SMSes.

This has been done for cost reasons (I guess?).

However, QA might actually want real SMSes to be sent for next and Staging so they can verify that the server generated dynamic links are correct. This can be done (ie. it's just configuration) but I'm not sure if we do want it or not.

@thescurry, what do you think?

(cc @mstidham)

thescurry commented 5 years ago

Let's turn it on for staging for sure.

bengtan commented 5 years ago

I talked with Bernard.

After communicating the reason for enabling it for Staging, he applied to same logic and decided to also enable it for next.

The server side PR is https://github.com/hippware/wocky/pull/2230

There is, however, a caveat.

SMSes will only be sent if the user is onboarding from a firebase number.

If the user account is a bypass number/account, SMSes will not be sent (but ignored instead).

mstidham commented 5 years ago

I created a new bugsnag ticket for the issue I saw while testing this. I'm unable to get contacts to load, the spinner spins once and then freezes. Should I expect this to load my contacts at this time? #3344

southerneer commented 5 years ago

@mstidham it should work so that's definitely a bug.

mstidham commented 5 years ago

Codepush Staging-Eric worked to get contacts to populate.

Issue #1 - Invite button doesn't seem to do anything. (Sent button doesn't show) also a bugsnag was created when I tapped Invite. #3346 Issue #2 - Phone numbers are not displaying. Issue #3 - Previous friends are not showing "Friends" button. Issue #4 - Should contacts be in alphabetical order? Issue #5 - Some contacts are blank, no name or number. I also see

Friends Button not displaying image

Missing Contact name & numbers: snip20190214_10

Missing Avatar: snip20190214_11

irfirl commented 5 years ago

I think contacts should be in alphabetical order for first name so users can find them easily. We should also omit contacts that have both blank first and last names (but keep ones that have one or the other) We should also omit users who are already friends.

southerneer commented 5 years ago

OK, I took this mock to mean that we were showing friends.

Makes sense to omit if no first or last name.

~In terms of alphabetical order, what if they have a last name but not a first name?~ For alpha sorting I'll just combine the first and last name and sort by that...should cover all cases.

southerneer commented 5 years ago

@mstidham I just codepushed 'more Invite Friends fixes'. I realize you're probably done for the day, but if you could just download that codepush and allow it to grab your contacts I've added a temporary Mixpanel event to collect that data so I can figure out what's bonking with your particular dataset.

@mstidham I think your first 2 problems are related...if phone #s aren't populating then the SMS invite won't work. I haven't been able to repro those problems yet.

As for the rest I'm filtering out existing friends, sorting alphabetically, and filtering out contacts with no first or last name.

mstidham commented 5 years ago

Codepush Staging-Eric "more Invite Friends fixes"

@southerneer Contacts were blank after installing that codepush.

image

southerneer commented 5 years ago

Ha! My job here is done.

Ok, I've got the data from Mixpanel. I'll run it on my end and try to figure out what else needs fixing.

southerneer commented 5 years ago

@mstidham I think I have the issues figured out. 1) I messed up the filtering code...it should be fixed now. 2) You only have 2 contacts in that phone with associated phone #s.

I codepushed again ('Invite Friends: fix contact filtering') and now you should see 2 results. Those you should be able to attempt an SMS invite. Or maybe you want to load up some more valid phone numbers for testing and then try the new codepush.

southerneer commented 5 years ago

I just ran a test with the latest codepush and was able to send an SMS invite to @irfirl .

mstidham commented 5 years ago

@southerneer I have tons of contacts on that account so I'm seeing way more than just 2. I have tapped invite on Don (has Staging) & Ally (doesn't have Staging) and they have not received a SMS invite. I'm also not seeing the phone number still.

image

mstidham commented 5 years ago

@bernardd I'm unable to send/receive sms with invite and @southerneer is able to send them just fine. Could this possibly be due to an issue with phone carrier through our sms services? I use Pioneer/Verizon.

toland commented 5 years ago

I think the inability to send/receive SMS is due to the fact that the Overseer test that tests our connectivity with Twilio has used up all of our capacity. I have disabled the test and we can regroup on this issue on Monday.

bernardd commented 5 years ago

Nah, that's not it - the limit we hit was on the particular test number I was using, and was a wocky feature not a Twilio one. However it's possible that, if you've been testing it a lot, you've hit the same limit for your user. @mstidham What's your user handle?

mstidham commented 5 years ago

I have used @miranda and @don.

bernardd commented 5 years ago

Okay, neither of them have actually got any successful sends marked against them, so that's not it.

bernardd commented 5 years ago

There's no traffic logs for either @miranda or @don that show the friendBulkInvite call, so whatever's going on, it's not getting as far as the server from what I can see.

southerneer commented 5 years ago

@mstidham (and anyone else QAing) I can't figure out why phone numbers aren't populating in the screenshots above. I've pulled the contacts datasets from the different phones you're testing with and used those datasets to test locally and the flow works as expected. Here's an example with one of the phone contact datasets:

image

The only difference is I'm using my own Firebase account and not Miranda's. So I've introduced the nuclear option via Codepush on StagingEric: 'Invite Friends nuclear option'. A few caveats:

  1. This is rigged to always show the Find Friends slide of Onboarding. If you want to run a new test, just kill and reload and it'll take you right back to that slide.
  2. In addition to the contacts dataset I've also added a temporary Mixpanel event (called contactsBulkResult) to pull the dataset returned from our backend with phone->account matches based on your account. This should allow me to mimic fully what's going on for anyone still experiencing problems while testing this flow.
bengtan commented 5 years ago

@southerneer:

FWIW, I tested the appearance of the 'Find Friends' screen on my phone (4.1.2 and the 'nuclear option' codepush) and it looks fine.

For 4.1.2, phone numbers are populated. There was a contact that appeared that didn't have a phone number associated (It was a facetime email-only contact).

But when I used the 'nuclear option' codepush, the facetime email-only contact didn't appear.

So, overall, the Find Friends screen is working fine for me.

I didn't go as far as to actually press 'Invite' nor 'Connect' though.

bengtan commented 5 years ago

More information for future reference:

I didn't go as far as to actually press 'Invite' nor 'Connect' though.

I tried an 'Invite' button (codepush: 'Nuclear option') and received an sms with the following text: 

@bengtan has invited you to tinyrobot. Please visit https://tinyrobot.page.link/S8sNt97jemZRJNpZ6 to join.

So it seems to be working ... for me, at least.

Also, if anyone wants to view the decision flowchart for these links, append ?d=1 to the end ie. https://tinyrobot.page.link/S8sNt97jemZRJNpZ6?d=1

southerneer commented 5 years ago

Cool, thanks for the info @bengtan

mstidham commented 5 years ago

Verified on Staging Version: 4.1.3

thescurry commented 5 years ago

LGTM!

mstidham commented 5 years ago

I'm running into a couple of issues with this ticket on Prod Version: 4.15.0. I have created a new ticket.

3880