Invite friend Functionality - Issue #36

[sulaimonlasisi]

@danfstucky This branch lets users invite LoL users that are not yet on LeagueSpec. It validates user input and prevents submission if validation criteria is not met. Two places where the functionality is accessible are:

1. When you search for an unregistered LoL summoner from the navbar
2. From the LeagueSpec Social drop-down in the navbar. Let me know if you have any questions/issues.

[danfstucky]

Ideally, If I search for a summoner and then click "Invite Friend", that should be all I have to do as a user to initiate a friend request. I'm guessing you at least need the user's email address though since I'm assuming LoL does not release people's emails as part of their api. At the very least though, you should not have to reenter the name of the summoner you just searched for to send a friend request to. The name confirmation is also unnecessary.

I think this form would be best as a modal that pops up when you click "Invite Friend". It only needs one field, the email address of the person you want to send the invite to. This will also eliminate a big chunk of your javascript, which I am avoiding looking at right now because I hate js Bootstrap has a simple modal tool you should look into

[sulaimonlasisi]

While I think this is mostly a design/user experience issue, below is my argument for structuring the page like I did: I agree with making the invitation a modal and I partially agree with not doing name confirmation. Partially agree because if someone typed a name wrong the first time, they have an opportunity to correct it. You don't want to have our server do the work of looking for a summoner name that does not exist or is not what the user intended. Also, about the email field being only the needed field, what if the email address being sent to does not play LoL? We shouldn't be sending messages to people that have no business with LoL in the first place. Summoner name check verifies that the person we are sending an email to has a business getting the email in the first place.

On Mon, Sep 19, 2016 at 8:52 PM, Dan Stucky notifications@github.com wrote:

Ideally, If I search for a summoner and then click "Invite Friend", that should be all I have to do as a user to initiate a friend request. I'm guessing you at least need the user's email address though since I'm assuming LoL does not release people's emails as part of their api. At the very least though, you should not have to reenter the name of the summoner you just searched for to send a friend request to. The name confirmation is also unnecessary.

I think this form would be best as a modal that pops up when you click "Invite Friend". It only needs one field, the email address of the person you want to send the invite to. This will also eliminate a big chunk of your javascript, which I am avoiding looking at right now because I hate js

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/danfstucky/LeagueSpec/pull/48#issuecomment-248171571, or mute the thread https://github.com/notifications/unsubscribe-auth/AKii_zK2gv1mjD6VX8ZIB1gIQxXlc2vhks5qry45gaJpZM4J1YsD .

[danfstucky]

@sl7v3 Correct me if I'm wrong, but isn't the only way to send a person an invite, by searching for them in app's search bar first? That search only returns results for LoL players so you would never be able to initiate an invite request with a person who does not play LoL or whose name does not exist anyway.

@danfstucky We can also invite a user by clicking on the LeagueSpec Social drop-down in the navbar and clicking on invite user. I included this feature as #2 of the items implemented in this pull request

[danfstucky]

Also, could you leave a brief comment explaining how you are testing these changes locally? It would help me for testing your code on my machine.

@danfstucky To test this, 1) Search for a user that is not on LeagueSpec, after the result appears, then click on the invite friend button. Once redirected to the invitation page, you will input summoner name and confirm it. If both are correct (testing the JS here), LeagueSpec lets you start the invitation process by checking whether that user exists, whether they are already on LeagueSpec or not. If the user is not already on LeagueSpec and user is a LoL player, fields appear for you to send the user an email. Once the input for the email field is valid and matches its confirmation (another JS test here), you will be able to send user an email. To further test this on the email receiver's end, receiver's email link should direct them to LeagueSpec sign up page. 2) Click on LeagueSpec Social in the navbar and then follow all the steps from second paragraph in 1 above.

[danfstucky]

I should have read your comment when you made the pull request. Since it is possible to invite people through the social drop down, I agree with you that the summoner name should be included. However, confirmations are generally only ever used for passwords and that's mainly because the user can't even see what they are typing when creating a password. I think the workflow here should be: 1) Enter the summoner's name once 2) Click initiate invitation 3) Server then verifies that this is an acceptable person 4) If it is acceptable, then display the email field and user enters the email address once. 5) Click Send invitation

The other client side validation that you have is great. I just think both confirmations should be removed.

@danfstucky fixed here https://github.com/danfstucky/LeagueSpec/commit/3d5e3574cb70f7a2a5eebea7ce8390a5ee908f81

[danfstucky]

Alright, I think the code you have now is fine. However, this branch does not contain any of the refactoring I had done earlier, meaning you must have created it before I pushed those commits to master.

There are definitely merge conflicts with master so I would like you to do a rebase so I can review your conflict resolution before pushing to master. We haven't been reviewing merge conflict changes yet, but technically every one should be reviewed and I want to make sure you don't revert a lot of the refactoring I did while merging. This is a very useful thing to learn how to do in git too. I would look at google first to see what a git rebase is, but it will basically allow us to move all of your commits on top of the ones already present in master without having to change master. I actually just combined all your commits related to this branch into a single one so the process will be a bit easier. The steps to do it in this case would be:

1) Make sure you pull down the changes I just made for your branch. I didn't actually touch any code but I did alter the git history which your branch needs to reflect before you do anything. You can see how I changed the history: (I combined all of your commits into a single one on the same branch). 2) Make sure your local master is completely up to date. 3) Checkout your issue branch, and enter git rebase origin/master -i. 4) This command will take you to a vim editor showing all the commits on your branch. Since I squashed your commits, you should only see 1. Enter :wq to save and exit that screen. If you had multiple commits here, you could pick and choose which to include, delete, squash, etc. 5) This will then try to apply the rebase, which will of course fail because of merge conflicts. Resolve the merge conflicts like you normally would. 6) Add your conflict resolution changes to git staging: git add -A. Once all conflicts are resolved, you can enter git rebase --continue to continue the rebase from where it left off and it should take you to another vim screen if there are no more conflicts. 7) This screen contains all your commit messages that are being rebased, as well as a list of the files that have been changed. Again, there should only be one commit message (at the very top) here. If I hadn't combined all your commits, you would see all the messages and have to choose which one you wanted to use for the rebase. You would just be choosing the commit's message, not it's content. 8) Enter :wq to save and exit again. 9) The rebase should finish executing. 10) Now you need to push your changes back up to your branch (NOT MASTER). Since you rewrote history, you will have to do a force push (Pretty much the only time this is a good idea). git push origin branch-name --force 11) Bam! Your rebase is done and you should now be able to see the updated history in your branch's commit history. I can also review your merge conflict changes in the current review.

I know that's a ton of information. Let me know if you get stuck on something.

[sulaimonlasisi]

@danfstucky That was very detailed and easy to follow, thanks! The pull request is now available for review.

[danfstucky]

+1 I will let you go ahead and merge to master first, but I recommend not starting on a new issue until I merge my changes as well. That way you have the most up to date code.

[sulaimonlasisi]

Sounds good! Merged to master here: https://github.com/danfstucky/LeagueSpec/commit/fbde8f3c97c1357676e695455cc387c2da1a082c