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

WIP: Exercise 4 #7

Closed
wants to merge 6 commits into from
Closed

WIP: Exercise 4 #7

wants to merge 6 commits into from

Conversation

ilonae
Copy link
Collaborator

@ilonae ilonae commented Nov 28, 2020

We have reviewed this pull request

Detail:
- change apollo-datasource-rest with apollo-datasource
- rewrite class Post
- update votePost
- update schema
- add context
- update server.js: add JWT, middleware, g-shield
- exercise-4.md

Completed:
For an implemented signup feature.

⭐ For a software-tested signup feature.

⭐ For password validation.

⭐ For unique email address validation.

⭐ For not having security issues according to instructions.
@ilonae ilonae changed the title Add JWT, Graphql-middleware, Graphql-shield, signup, bcrypt, dotenv WIP: Exercise 4 Nov 28, 2020
@ilonae ilonae marked this pull request as draft November 28, 2020 13:16
"apollo-datasource-rest": "^0.9.5",
"apollo-datasource": "^0.7.2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yiimnta Danke, dass du das geändert hast!

ilonae and others added 2 commits December 2, 2020 14:08
- Removing this.context in PostDatasource
- Adding currUser in Context
Copy link
Contributor

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

This was a suggestion given live in the seminar. So no "Approve" or "Request Changes" here.

backend/src/context.js Outdated Show resolved Hide resolved
backend/src/resolver.js Outdated Show resolved Hide resolved
backend/src/DataSources/users-data-source.js Outdated Show resolved Hide resolved
backend/src/DataSources/users-data-source.js Outdated Show resolved Hide resolved
backend/src/DataSources/users-data-source.js Show resolved Hide resolved
backend/src/token.js Outdated Show resolved Hide resolved
Copy link
Contributor

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

it's good

Here's your intermediate review:

It's good! I think you will receive all the ⭐ when you follow my suggestions. Just make sure that you don't commit your real .env, maybe rebase on origin/main once again. All in all, your code just needs a larger refactoring and cleanup.

@@ -0,0 +1 @@
JWT_SECRET=thecountryroads
Copy link
Contributor

Choose a reason for hiding this comment

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

❌ If this stays, you will loose a ⭐ here.

⭐ For not having security issues according to instructions.

You have multiple options:
a) Mock jsonwebtoken node module in your tests, see our forum our group lichtow: Systems-Development-and-Frameworks/lichtow@6d164b9#diff-45f14d66b6a3b7ed6473601b04ce8c30013d014dfd73e439376d93e9196f93dcR98
b) Use dotenv-flow and commit a non-secret in .env.test
c) Explain in README.md one has to create a .env file before running the tests.

backend/src/DataSources/posts-data-source.js Outdated Show resolved Hide resolved
backend/src/DataSources/posts-data-source.js Show resolved Hide resolved
backend/src/DataSources/posts-data-source.js Outdated Show resolved Hide resolved
backend/src/DataSources/posts-data-source.js Show resolved Hide resolved
backend/src/DataSources/users.spec.js Show resolved Hide resolved
backend/src/context.js Show resolved Hide resolved
backend/src/resolver.js Show resolved Hide resolved
backend/src/server.js Show resolved Hide resolved
backend/src/utils.js Show resolved Hide resolved
backend/src/DataSources/posts-data-source.js Show resolved Hide resolved
backend/src/DataSources/posts-data-source.js Outdated Show resolved Hide resolved
backend/src/DataSources/posts-data-source.js Show resolved Hide resolved
decoded = {}
})

const testContext = () => decoded

Choose a reason for hiding this comment

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

Check this suggestion from Robert

@ilonae
Copy link
Collaborator Author

ilonae commented Dec 8, 2020

Thank you very much for your helpful reviews @roschaefer, @MaykAkifovski and @ubiquitousbyte ! We're working through this. 💪

@aloparev aloparev removed the request for review from medizinmensch December 9, 2020 12:30
@aloparev aloparev closed this Dec 15, 2020
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.

None yet

6 participants