The Wayback Machine - https://web.archive.org/web/20201203080844/https://github.com/jsperf/jsperf.com/pull/467
Skip to content

/ jsperf.com

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

Jump to bottom

Use GitHub login info to associate pages with a user #467

Merged
merged 3 commits into from Oct 9, 2018
Merged

Use GitHub login info to associate pages with a user #467

merged 3 commits into from Oct 9, 2018

Conversation

@swang
Copy link
Contributor

@swang swang commented Mar 14, 2018

Addresses #312,

This changes everything so that GitHub usernames will be the indexed/associated element of all pages/tests, rather than the loose lookup of the author's name or the current user session to tie the users pages together.

These changes unfortunately won't allow users to associate their old tests with their GitHub accounts. It is possible to create a "retrieval" of their tests. By sending an email out then asking them which GitHub username they want their changes to be associated with, then checking to see that the GitHub username's email address matches. And if the addresses match, fill the authorGitHub property with said GitHub username. I don't know exactly how many people would want to get their old tests back.

Things this PR does not change that should be addressed before merging:

  • Users can still set their name and email address, although their GitHub username still gets associated with their pages/comments. Not sure if you want to just hardcode this to whether gets returned from GitHub.
  • Gravatar is still being used rather than using GitHub's avatar.
  • The logic of only being able to edit the first revision if you keep the same session is kept for now. I'm not sure if we want to change this to be associated with GitHub usernames, and whether or not we want users to only edit the initial revision or any subsequent revisions they may have. I'm not sure about the initial reason for the design choice, so probably need more information before I make changes to this.
  • Probably some other stuff I can't remember...

Few small nitpicks to consider:

  • I used authorGitHub, better if its authorGithub or something completely different like githubUsername?
  • Should the authorGitHub field (or whatever it ends up being called) by set to NOT NULL? The reason I did this was to make sure it returned an error if there was no GitHub username provided and make it easier to spot problems (rather than making it default to '', which would silently allow for errors). However, I could see issues maybe with older pages/tests...

Also I can't seen to run e2e tests. Seems like I need a saucelabs account and provide by GitHub username/password? I'm sure these need to be updated or they won't pass. Would appreciate assistance in setting that up or having those tests fixed up if they're broken.
@swang
Use github login info to associate pages/comments with a user
Loading status checks…
23e3d49
@swang swang force-pushed the swang:use_github branch from 24baa92 to 23e3d49 Mar 14, 2018
@mathiasbynens
mathiasbynens approved these changes Mar 14, 2018
View changes
Copy link
Contributor

@mathiasbynens mathiasbynens left a comment

LGTM with the follow-up changes you suggested!
@mathiasbynens
Copy link
Contributor

@mathiasbynens mathiasbynens commented Mar 14, 2018

Thank you for tackling this!!!

Users can still set their name and email address, although their GitHub username still gets associated with their pages/comments. Not sure if you want to just hardcode this to whatever gets returned from GitHub.

Just re-using the GitHub values seems sensible IMHO. Let’s do that!

Gravatar is still being used rather than using GitHub's avatar.

No strong opinion here.

The logic of only being able to edit the first revision if you keep the same session is kept for now. I'm not sure if we want to change this to be associated with GitHub usernames, and whether or not we want users to only edit the initial revision or any subsequent revisions they may have. I'm not sure about the initial reason for the design choice, so probably need more information before I make changes to this.

The intention is that users can edit any revision they created themselves within the same session. This is what jsPerf v1 did.

I’d be fine with giving the same GitHub user the ability to change these revisions indefinitely with this change if we can base it on some kind of GitHub user ID rather than user name (which might be re-used in case of deleted accounts).

I used authorGitHub, better if its authorGithub or something completely different like githubUsername?

authorGitHub sounds good to me.

Also I can't seen to run e2e tests. Seems like I need a saucelabs account and provide by GitHub username/password? I'm sure these need to be updated or they won't pass. Would appreciate assistance in setting that up or having those tests fixed up if they're broken.

@maxbeatty, can you help?
@maxbeatty
Copy link
Member

@maxbeatty maxbeatty commented Mar 18, 2018

Thanks for putting this together! I don't have any strong opinions about outstanding issues.

e2e tests can be run locally by starting a selenium server locally. If you're familiar with docker, this should work:

docker run -d -p 4444:4444 --shm-size=2g selenium/standalone-chrome:2.53.1

Then, you would set environment variables to point at your running selenium server (note port from above 4444):

SELENIUM_SERVER=http://localhost:4444 \
E2E_GITHUB_USER=swang \
E2E_GITHUB_PASS=your-actual-password \
npm run test-e2e

e2e tests were removed from CI because of their instability. My experience with GoogleChrome/puppeteer has been much more positive than with selenium. Not to suggest that it should be addressed in this PR, but that would remove the dependency on SauceLabs and be easier to run locally.
maxbeatty added 2 commits Oct 9, 2018
@maxbeatty
Merge branch 'master' into use_github
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
96e7271
@maxbeatty
Merge branch 'master' into use_github
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
aaf259a
@maxbeatty maxbeatty merged commit 02295f7 into jsperf:master Oct 9, 2018
3 checks passed
3 checks passed
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deployment/now Deployment has completed
Details
@maxbeatty maxbeatty mentioned this pull request Oct 17, 2018
Closed
0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathiasbynens mathiasbynens

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@swang @mathiasbynens @maxbeatty
You can’t perform that action at this time.