-
Notifications
You must be signed in to change notification settings - Fork 222
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
Interface elements to support Github personal access tokens #2061
base: master
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThe recent update introduces functionality for integrating GitHub with user accounts. It allows users to update their GitHub access tokens directly through a dedicated form. This integration is facilitated by adding new actions in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
<% @title = "Github Access Token" %> | ||
|
||
<h2>Github Update Access Token</h2> | ||
<% if GithubIntegration.connected %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional check GithubIntegration.connected
is used to determine if the GitHub integration is connected. However, this might not correctly reflect the individual user's connection status since it seems to be checking a class-level property rather than an instance-level property related to the current user.
Consider modifying this to check the GitHub integration status for the specific user instance to ensure accurate display logic.
<div class="input-field"> | ||
<%= form.text_field :access_token, required: true %> | ||
<%= form.label :access_token, "Access Token" %> | ||
<span class="helper-text">Helpful description here</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper text under the access token input field is generic ("Helpful description here"). It would be beneficial to provide a more descriptive message to guide the user on where to find their GitHub access token or why it's needed.
Consider updating the helper text to provide clear instructions on obtaining a GitHub personal access token and its intended use.
action_auth_level :github_token_form, :student | ||
def submit_github_token_form | ||
github_integration = GithubIntegration.find_by(user_id: @user.id) | ||
access_token = params[:access_token] | ||
|
||
if github_integration.nil? | ||
github_integration = GithubIntegration.create!(access_token:, user: @user) | ||
else | ||
github_integration.update!(access_token:) | ||
end | ||
|
||
flash[:success] = "Updated Github Token" | ||
redirect_to(user_path) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The submit_github_token_form
action properly handles both the creation of a new GithubIntegration
instance for the user if one does not already exist, and the updating of an existing instance with the new access token. The use of create!
and update!
methods ensures that any errors during these operations will raise exceptions, which is appropriate for catching unexpected issues.
However, there's no explicit handling of potential exceptions that could arise from the create!
or update!
operations, which could lead to a poor user experience if the database operations fail.
Consider adding error handling around the database operations to gracefully handle any exceptions and provide meaningful feedback to the user.
Thank you for the PR - we'll take a look at it and reach out if we have questions! (if you could give us permissions to push to your fork, we'll be able to make changes directly to this branch) |
Added interface button in Account page next to the "Connect to Github" button - "Use Personal Access Token". Clicking takes to a single field form to manually (copy-paste) token. This uses the same tables as the existing github_integration and applies the same encryption through ActiveRecord.
Note: you can set the token without progressing through the Oauth flow so this avoids having to approve the permissions per Issue #2059 .
There are no tests so understand if you dont Accept the PR, but it should be proof of concept enough to allow others to easily implement the feature fully.
Summary
Summary generated by Reviewpad on 26 Jan 24 01:14 UTC
This pull request adds a button to set a Github token for user authentication. It includes changes to the users controller, views, and routes. Specifically, it adds methods for showing and submitting a Github token form, as well as updating the Github integration with the new token. This allows users to use their personal access token for Github interactions.
Description
Motivation and Context
How Has This Been Tested?
No, see above.
Types of changes
Checklist:
overcommit --install && overcommit --sign
to use pre-commit hook for lintingOther issues / help required
If unsure, feel free to submit first and we'll help you along.