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.

Sign up for GitHub

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

Jump to bottom

Interface elements to support Github personal access tokens #2061

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Interface elements to support Github personal access tokens #2061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions app/controllers/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ class UsersController < ApplicationController
skip_before_action :update_persistent_announcements
before_action :set_gh_oauth_client, only: [:github_oauth, :github_oauth_callback]
before_action :set_user,
only: [:github_oauth, :github_revoke, :lti_launch_initialize,
:lti_launch_link_course]
only: [:github_oauth, :github_revoke,
:show_github_token_form, :submit_github_token_form,
:lti_launch_initialize, :lti_launch_link_course]

# GET /users
action_auth_level :index, :student
Expand Down Expand Up @@ -250,6 +251,27 @@ def lti_launch_link_course
redirect_to(course)
end

action_auth_level :github_token, :student
def show_github_token_form
@github_integration = GithubIntegration.find_by(user_id: @user.id)
render('github_token')
end

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
Comment on lines +260 to +273
Copy link
Contributor

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.


action_auth_level :github_oauth, :student
def github_oauth
github_integration = GithubIntegration.find_by(user_id: @user.id)
Expand Down
13 changes: 13 additions & 0 deletions app/views/users/github_token.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<% @title = "Github Access Token" %>

<h2>Github Update Access Token</h2>
<% if GithubIntegration.connected %>
Copy link
Contributor

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.

<%= form_with url: github_token_user_path do |form| %>
<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>
Copy link
Contributor

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.

</div>
<%= form.submit "Update Token", { class: "btn primary" } %>
<% end %>
<% end %>
7 changes: 7 additions & 0 deletions app/views/users/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
</span>
<% end %>
<% if GithubIntegration.connected %>
<h4>Github Settings</h4>
<% if @user.github_integration && @user.github_integration.is_connected %>
<%= link_to github_revoke_user_path(@user), data: { method: "post" } do %>
<span class="btn primary">
Expand All @@ -75,6 +76,12 @@
</span>
<% end %>
<% end %>

<%= link_to github_token_user_path(@user) do %>
<span class="btn primary">
Use Personal access Token
</span>
<% end %>
<% end %>
<h4>API Settings</h4>
<%= link_to device_flow_activation_path do %>
Expand Down
2 changes: 2 additions & 0 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@
get "lti_launch_initialize", on: :member
post "lti_launch_link_course", on: :member
post "github_revoke", on: :member
get "github_token", on: :member, to: 'users#show_github_token_form'
post 'github_token', on: :member, to: 'users#submit_github_token_form'
get "github_oauth_callback", on: :collection
match "update_password_for_user", on: :member, via: [:get, :put]
post "change_password_for_user", on: :member
Expand Down
Loading