Skip to content
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

index user if not existed and log last active time for user accessing apis via backend apps #227

Merged
merged 23 commits into from
Nov 8, 2020

Conversation

ztsai
Copy link
Contributor

@ztsai ztsai commented Oct 13, 2020

closes #220

@coveralls
Copy link

coveralls commented Oct 13, 2020

Coverage Status

Coverage decreased (-2.09%) to 85.947% when pulling c584d5b on backendUsers into c678c94 on master.

@MrOrz MrOrz self-requested a review October 13, 2020 15:50
@ztsai ztsai marked this pull request as draft October 15, 2020 05:16
@MrOrz MrOrz removed their request for review October 15, 2020 05:21
@ztsai ztsai marked this pull request as ready for review October 18, 2020 00:06
@ztsai ztsai requested a review from MrOrz October 18, 2020 00:06
Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 3 comments here first, still under review QQ

src/__tests__/auth.js Outdated Show resolved Hide resolved
src/graphql/models/User.js Show resolved Hide resolved
@@ -3,6 +3,7 @@ import client from 'util/client';
import CreateBackendUsers from '../createBackendUsers';
import fixtures from '../__fixtures__/createBackendUsers';
import { sortBy } from 'lodash';
jest.setTimeout(50000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to remove L8 to make this line work xd

Copy link
Contributor Author

@ztsai ztsai Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, good catch.
Actually, I think this might only be needed for tests to pass locally. It doesn't seem to timeout as easily on travis.
I am actually thinking, maybe we can make timeout value an environment variable to be set in .env and move this to setup since this is pretty much environment performance dependent (I don't know about everyone else, but they run way slower on my laptop than on travis :p)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK that we increase timeout directly.

Increasing timeout does not impact time spent on test, but can save precious developing energy on problems that we can really solve.

Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a million for looping in tests for index.js! This is the first time apollo server context logic can be included in tests. I am grateful for @ztsai 's integration effort 🙇‍♀️

I am a bit confused how Apollo server context could interact with other middlewares like checkHeaders. We may need to check if the current logic works for web browser users.

Lastly, I have some nitpicking suggestions are regarding snapshot names :P

src/graphql/models/__tests__/User.js Outdated Show resolved Hide resolved
src/graphql/mutations/__tests__/CreateOrUpdateUser.js Outdated Show resolved Hide resolved
src/graphql/mutations/__tests__/CreateOrUpdateUser.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/__tests__/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@ztsai
Copy link
Contributor Author

ztsai commented Oct 30, 2020

frontend users are tested with logging in from development frontend, and backend users are tested with

curl "http://localhost:5000/graphql?userId=$USERID" -H 'Accept-Encoding: gzip, deflate, br' -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'Connection: keep-alive' -H 'DNT: 1' -H 'Origin: http://localhost:5000' -H "x-app-secret:$SECRET" --data-binary '{"query":"query {\n  GetUser{\n  \tid\n  \tname\n    avatarUrl\n    level\n  }\n}"}' --compressed

Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix!

Added some nit-picking comments that should not be merge blockers. Feel free to adapt and merge anytime :)

src/graphql/models/User.js Outdated Show resolved Hide resolved
src/graphql/mutations/CreateOrUpdateUser.js Outdated Show resolved Hide resolved
Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the refactor! Let's merge this. I will try deploy it to staging before wednesday, along with data migration.

@MrOrz MrOrz merged commit 2c0769e into master Nov 8, 2020
@MrOrz MrOrz deleted the backendUsers branch November 10, 2020 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ad hoc backend user creation
3 participants