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

[MAGE] GH login + so many aMAGEing updates #1546

Merged
merged 18 commits into from
Nov 7, 2023
Merged

Conversation

vincanger
Copy link
Contributor

Description

  • put generations behind GH login
  • created a user dashboard page to view user's apps
  • check if user has starred our repo via GH api
  • update meta og:title to include MageGPT and Mage GPT
  • new twitter banner with updated name

image
image

Select what type of change this PR introduces:

  1. Just code/docs improvement (no functional change).
  2. Bug fix (non-breaking change which fixes an issue).
  3. New feature (non-breaking change which adds functionality).
  4. Breaking change (fix or feature that would cause existing functionality to not work as expected).

@testing-miho
Copy link

Hello from my test account 👋

It seems that the method for checking if a user starred the repo doesn't work 😢

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Awesome job @vincanger !

I think all is pretty solid, and nice job on doing all these changes! I left some comments on how to improve things, check them out.

wasp-ai/src/client/components/Header.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/components/Header.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/components/Header.jsx Show resolved Hide resolved
wasp-ai/src/client/components/Header.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/components/Header.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/pages/UserPage.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/pages/UserPage.jsx Outdated Show resolved Hide resolved
wasp-ai/src/server/operations.ts Outdated Show resolved Hide resolved
wasp-ai/src/server/operations.ts Outdated Show resolved Hide resolved
wasp-ai/src/server/operations.ts Outdated Show resolved Hide resolved
@Martinsos
Copy link
Member

Hello from my test account 👋

It seems that the method for checking if a user starred the repo doesn't work 😢

Oh wow, why not?

@vincanger
Copy link
Contributor Author

Awesome job @vincanger !

I think all is pretty solid, and nice job on doing all these changes! I left some comments on how to improve things, check them out.

I addressed all your comments and tried to clean up and extract as many things as I could. I think the header works a lot nicer now that it accepts children and the StatusPill as a dependency.

To address the GH api issue, we need to send the API the user's access token which we receive on successful login to GH via OAuth. Unfortunately, Wasp doesn't expose this token in any way for us to use, so that is out of the question.

@Martinsos
Copy link
Member

Awesome job @vincanger !
I think all is pretty solid, and nice job on doing all these changes! I left some comments on how to improve things, check them out.

I addressed all your comments and tried to clean up and extract as many things as I could. I think the header works a lot nicer now that it accepts children and the StatusPill as a dependency.

To address the GH api issue, we need to send the API the user's access token which we receive on successful login to GH via OAuth. Unfortunately, Wasp doesn't expose this token in any way for us to use, so that is out of the question.

Oh that is interesting. Is that a missing feature in Wasp? Sounds like it is something we should add? @infomiho you are the expert here, what are your thoughts? Should we create an GH issue for it, if we don't already have one?

@vincanger
Copy link
Contributor Author

ok @Martinsos, I added the return declaration to the UserPage, pushed the changes, and addressed all remaining comments.

@infomiho
Copy link
Contributor

Yes, we are not giving the developer access to the Github Access Token in the getUserFields function therefore they can't do API calls on the behalf of the user.

https://github.com/wasp-lang/wasp/blob/main/waspc/data/Generator/templates/server/src/auth/providers/oauth/init.ts#L46

It should probably be an issue, but I wouldn't add it "hey we need to support giving the access token to the user" I would do it inside of the overall auth effort of moving to https://github.com/wasp-lang/openid-client-experiment and see how we do it.

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

@vincanger I did another round of reviewing!

wasp-ai/src/client/components/Header.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/components/Header.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/stats/stats.js Outdated Show resolved Hide resolved
wasp-ai/src/client/pages/UserPage.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/pages/UserPage.jsx Outdated Show resolved Hide resolved
@vincanger
Copy link
Contributor Author

ok @Martinsos -- reviewd and updated. awaiting your final review :)

@vincanger
Copy link
Contributor Author

vincanger commented Nov 3, 2023

@Martinsos updated! I feel pretty confident that the naming of names was done in the name of clean code :)

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

@vincanger nice job with cleaning up the names -> we are not yet there completely I think, but you brought them close enough so I could understand what they are, which is already great! I left some suggestions on how to possibly improve them further + a few other small comments and one possible mistake.

Please go through all of these, also resolve any comments where my response was last and you are happy with it, and then I will take another look!

wasp-ai/src/client/pages/StatsPage.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/project/converters.js Outdated Show resolved Hide resolved
wasp-ai/src/client/project/converters.js Outdated Show resolved Hide resolved
wasp-ai/src/client/project/converters.js Outdated Show resolved Hide resolved
wasp-ai/src/client/project/converters.js Outdated Show resolved Hide resolved
wasp-ai/src/client/pages/UserPage.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/pages/UserPage.jsx Outdated Show resolved Hide resolved
wasp-ai/src/client/stats/stats.js Outdated Show resolved Hide resolved
wasp-ai/src/client/components/Header.jsx Show resolved Hide resolved
wasp-ai/src/client/pages/MainPage.jsx Outdated Show resolved Hide resolved
@vincanger
Copy link
Contributor Author

ok @Martinsos ! the renaming of renamed names is complete :)

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

@vincanger nice job, I think this is it!

I left a couple small comments, please give them a look and handle them as you wish, and then you can merge! I approved the PR.

wasp-ai/src/client/project/utils.js Outdated Show resolved Hide resolved
wasp-ai/src/client/project/utils.js Outdated Show resolved Hide resolved
wasp-ai/src/client/project/utils.js Outdated Show resolved Hide resolved
@vincanger vincanger merged commit 3dd6279 into wasp-ai Nov 7, 2023
4 checks passed
@vincanger vincanger deleted the vince-add-ghlogin-mage branch November 7, 2023 10:24
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