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

feat: rest-express-auth example #2869

Draft
wants to merge 22 commits into
base: latest
Choose a base branch
from
Draft

Conversation

gautvm
Copy link

@gautvm gautvm commented May 18, 2021

This is the PR where I am working on the rest-express-auth example, details #2838. This is currently a draft.

@gautvm gautvm marked this pull request as draft May 18, 2021 17:59
@gautvm gautvm changed the title New Prisma example - rest-express-auth feat: rest-express-auth example May 18, 2021
@ruheni ruheni self-requested a review June 12, 2021 09:39
@ruheni
Copy link
Contributor

ruheni commented Nov 22, 2021

Hi @eternalmoon1234 👋🏽

Do you have any updates on this PR? Would you like any help pushing this across the finish line? 🙂

@ruheni
Copy link
Contributor

ruheni commented Mar 31, 2022

Hey @eternalmoon1234 👋🏽

Do you intend to pick up this PR, and do you need any help?

@gautvm
Copy link
Author

gautvm commented Jul 16, 2022

Hey @ruheni, ✋
Yes, I do intend to pick up the PR and will finish it by the end of next week! Sorry for the inactivity with this, I've been coding on and off recently and I must say I forgot about this 😅

Looking forward to finishing this!

@gautvm gautvm marked this pull request as ready for review March 7, 2023 21:58
@gautvm gautvm requested a review from ruheni March 7, 2023 21:58
@gautvm
Copy link
Author

gautvm commented Mar 7, 2023

Hey @ruheni 👋

I finally finished up this PR. I've implemented the changes you proposed (as well as some others) and have followed the same standard of the rest-express-auth example.

Please review & propose any changes when you can!

Copy link
Contributor

@ruheni ruheni left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I've left some comments. The example seems to be missing a test for it. You can add one in the .github/tests folder.

One other thing I didn't include in the comments is that I think some endpoints might not require authentication, such as /feed and /post/:id (both GET endpoints). I would recommend protecting endpoints with write operations in general. Let me know if you would like any help determining what endpoints to protect

.github/readmes/typescript/rest-express-auth/README.md Outdated Show resolved Hide resolved
.github/readmes/typescript/rest-express-auth/README.md Outdated Show resolved Hide resolved
.github/readmes/_using-the-rest-api-auth.md Outdated Show resolved Hide resolved
typescript/rest-express-auth/README.md Outdated Show resolved Hide resolved
typescript/rest-express-auth/prisma/seed.ts Show resolved Hide resolved
@gautvm
Copy link
Author

gautvm commented Mar 20, 2023

Hey @ruheni 👋

I've incorporated all of your requested changes. As for the protected routes, I've modified the ones that I thought required protection based on your guidance, although please feel free to suggest any alterations if you'd propose something different.

Please let me know if there's anything else you'd like me to change 🙂

@gautvm gautvm requested a review from ruheni March 20, 2023 22:08
@gautvm
Copy link
Author

gautvm commented Mar 21, 2023

Do I need to update the rest test and postman collection now that there are protected routes?

@ruheni
Copy link
Contributor

ruheni commented Mar 22, 2023

Hey 👋🏾
I would recommend creating a new test and collection for it to get the tests to pass.

@gautvm
Copy link
Author

gautvm commented Apr 1, 2023

Sounds good, I'll go ahead and write the new test in.

@gautvm
Copy link
Author

gautvm commented May 16, 2023

Hey @ruheni 👋

The tests should pass now. Mind running the checks & verifying that everything's all good? If so, I think this is ready to merge.

@jharrell jharrell marked this pull request as draft December 19, 2023 15:14
@XprabhatX
Copy link

The logs for the failed test are no longer available. Can anyone specify what's the problem.

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.

4 participants