-
Notifications
You must be signed in to change notification settings - Fork 5
feature/ github oauth integration #561
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
base: main
Are you sure you want to change the base?
Conversation
use BetterTogether controller
Prevent visibility of the feature before it's ready
Allows for linking OAuth authorizations to Person and Platform records
Signed-off-by: Robert Smith <[email protected]>
Signed-off-by: Robert Smith <[email protected]>
Signed-off-by: Robert Smith <[email protected]>
Signed-off-by: Robert Smith <[email protected]>
Signed-off-by: Robert Smith <[email protected]>
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.
Pull Request Overview
This PR integrates GitHub OAuth into Community Engine and scaffolds new CRUD interfaces and tests for PersonPlatformIntegration and Seed models.
- Adds empty view specs for seeds and authorizations to improve test coverage.
- Introduces
PersonPlatformIntegrationControllerwith matching views and routes. - Implements model logic in
PersonPlatformIntegrationfor handling tokens, but contains a minor bug. - Updates helper documentation and corrects a small typo.
Reviewed Changes
Copilot reviewed 82 out of 86 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| spec/views/better_together/seeds/show.html.erb_spec.rb | Empty test stub—needs render and assertions |
| spec/views/better_together/person_platform_integrations/show.html.erb_spec.rb | Empty test stub—needs render and assertions |
| app/controllers/better_together/person_platform_integrations_controller.rb | Incorrect instance variable used in create action |
| app/models/better_together/person_platform_integration.rb | Class method references undefined variable in attributes_from_omniauth |
| app/helpers/better_together/person_platform_integrations_helper.rb | Spelling typo in module documentation |
Comments suppressed due to low confidence (11)
spec/views/better_together/seeds/show.html.erb_spec.rb:11
- Implement the
rendercall and add expectations (e.g.,expect(rendered).to match(...)) so this view spec actually verifies the output.
# render
spec/views/better_together/seeds/new.html.erb_spec.rb:11
- Uncomment and use
renderplusassert_selectassertions to verify the form elements are present.
# render
spec/views/better_together/seeds/index.html.erb_spec.rb:14
- Add
renderand assertions (e.g.,assert_select) to verify each seed is rendered in the list.
# render
spec/views/better_together/seeds/edit.html.erb_spec.rb:15
- Use
renderandassert_selectto confirm the edit form targets the correct path and fields.
# render
spec/views/better_together/person_platform_integrations/show.html.erb_spec.rb:11
- Call
renderand then assert that the key attributes (provider, uid, etc.) actually appear in the output.
# render
spec/views/better_together/person_platform_integrations/new.html.erb_spec.rb:11
- Uncomment and implement the
assert_selectblocks afterrenderto validate the new integration form fields.
# render
spec/views/better_together/person_platform_integrations/index.html.erb_spec.rb:11
- Add a
rendercall and useassert_selectto check that each integration is displayed correctly.
# render
spec/views/better_together/person_platform_integrations/edit.html.erb_spec.rb:15
- Include
renderandassert_selectto ensure the edit form is rendered with correctactionand input fields.
# render
app/controllers/better_together/person_platform_integrations_controller.rb:27
- The
createaction initializes@better_together_person_platform_integrationbut then calls@person_platform_integration.save. Rename the variable or call@better_together_person_platform_integration.saveto avoid nil errors.
@better_together_person_platform_integration = BetterTogether::PersonPlatformIntegration.new(person_platform_integration_params)
app/models/better_together/person_platform_integration.rb:82
attributes_from_omniauthis a class method and referencesperson_platform_integration, which is undefined. Replace this check withif auth.persisted?or move it into an instance context.
attributes[:profile_url] = auth.info.urls.first.last unless person_platform_integration.persisted?
app/helpers/better_together/person_platform_integrations_helper.rb:4
- Fix typo: change
conainstocontains.
# This module conains helper methods for PersonPLatformIntegrations
Signed-off-by: Robert Smith <[email protected]>
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.
This needs to be removed
rsmithlal
left a comment
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.
needs a lot of cleanup to remove the commits from the other feature branches
| class PagesController < FriendlyResourceController # rubocop:todo Metrics/ClassLength | ||
| before_action :set_page, only: %i[show edit update destroy] | ||
|
|
||
| skip_before_action :check_platform_setup, unless: -> { ::BetterTogether::Platform.where(host: true).any? } |
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.
Update check to use host scope instead of where query
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.
This needs to be removed or moved to seed branch
| def locale_options_for_select(selected_locale = I18n.locale) | ||
| options_for_select( | ||
| I18n.available_locales.map { |locale| [I18n.t("locales.#{locale}", locale:), locale] }, | ||
| I18n.available_locales.map { |locale| [I18n.t("better_together.languages.#{locale}", locale:), locale] }, |
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.
Check this key
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.
remove this
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.
check these keys
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.
remove and/or move to seeds branch
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.
remove and/or move to seeds branch
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.
remove and/or move to seeds branch
| spec.add_dependency 'activerecord-postgis-adapter' | ||
| spec.add_dependency 'active_storage_svg_sanitizer' | ||
| spec.add_dependency 'active_storage_validations' | ||
| spec.add_dependency 'acts_as_tenant' |
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.
remove this change
| active_storage_validations | ||
| activerecord-import | ||
| activerecord-postgis-adapter | ||
| acts_as_tenant |
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.
remove
| # PATCH/PUT /seeds/1 | ||
| def update | ||
| if @seed.update(seed_params) | ||
| redirect_to @seed, notice: 'Seed was successfully updated.', status: :see_other |
Check notice
Code scanning / Brakeman
Possible unprotected redirect. Note
| @@ -0,0 +1,10 @@ | |||
| <p style="color: green"><%= notice %></p> | |||
|
|
|||
| <%= render @person_platform_integration %> | |||
Check notice
Code scanning / Brakeman
Render path contains parameter value. Note
| @@ -0,0 +1,10 @@ | |||
| <p style="color: green"><%= notice %></p> | |||
|
|
|||
| <%= render @seed %> | |||
Check notice
Code scanning / Brakeman
Render path contains parameter value. Note
Summary of Changes
This pull request adds GitHub OAuth Integration to the Community Engine, enabling users to sign in using their GitHub accounts. It also introduces a new
PersonPlatformIntegrationmodel to track external platform connections (starting with GitHub), along with supporting views, controllers, and tests.Key Changes
1. OAuth Support for GitHub Sign-In
omniauth,omniauth-github, andomniauth-rails_csrf_protectiongems.omniauthablewith GitHub as a provider.OmniauthCallbacksControllerto handle GitHub authentication flow.2. New PersonPlatformIntegration Model
Personand external platforms like GitHub.uid,access_token,profile_url).3. GitHub API Client
BetterTogether::Githubclass for interacting with GitHub’s API using app installation tokens.4. New CRUD Interface for Managing Integrations
PersonPlatformIntegrationsControllerwith views.5. Updated User Model
Useris nowomniauthable, with ahas_many :person_platform_integrationsassociation.6. Migration and Database Changes
better_together_person_platform_integrationstable.7. Gem and Dependency Updates
redis,rubocop, andaws-sdkversions as part of routine updates.Affected Files
Why These Changes?
This is part of ongoing work to improve developer experience and make it easier for contributors to link their Community Engine accounts to external platforms. The GitHub integration lays the groundwork for:
Testing
PersonPlatformIntegrationsController.Migration Required
✅ Run
bin/rails db:migrateto apply the newbetter_together_person_platform_integrationstable.