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

TAN-3350 - Refactor sso & verification methods with clearer structure and more code sharing #9843

Open
wants to merge 20 commits into
base: TAN-3350-bring-verification-into-core
Choose a base branch
from

Conversation

jamesspeake
Copy link
Contributor

@jamesspeake jamesspeake commented Dec 18, 2024

  • New IdMethod module as the root for all omniauth & verification methods
  • Both omniauth and verification are now both included in the same way as modules instead of omniauth using class inheritance
  • all_methods is populated with just one call and then AuthenticationService and VerificationService filter this list based on verification? or auth?
  • Moved google, facebook, azuread and azureadb2c into engines for consistency

Changelog

Technical

  • Refactor sso & verification methods with clearer structure and more code sharing

Copy link

@jamesspeake jamesspeake changed the base branch from master to TAN-3350-bring-verification-into-core December 18, 2024 12:16
@cl-dev-bot
Copy link
Collaborator

cl-dev-bot commented Dec 18, 2024

Messages
📖 Changelog provided 🎉
📖 Notion issue: TAN-3350
📖

Run the e2e tests

📖 Check translation progress

Generated by 🚫 dangerJS against 632e41a

@jamesspeake jamesspeake marked this pull request as ready for review January 3, 2025 16:27
@jamesspeake jamesspeake requested a review from adessy January 3, 2025 16:28
Copy link
Contributor

@adessy adessy left a comment

Choose a reason for hiding this comment

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

Looks great! I think there’s still some room to simplify and clarify the class/module hierarchy, but it’s heading in the right direction 😊

def all_methods
self.class.all_methods
IdMethod.all_methods.filter_map { |k, v| [k, v] if v.auth? }.to_h
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IdMethod.all_methods.filter_map { |k, v| [k, v] if v.auth? }.to_h
IdMethod.all_methods.select { |_, v| v.auth? }

@@ -30,7 +30,7 @@
# the state of the request. Especially large strings are stored in fixtures.
describe OmniAuth::Strategies::AzureActiveDirectory do
let(:app) { ->(_) { [200, {}, ['Hello world.']] } }
let(:x5c) { Rails.root.join('spec/fixtures/azure_active_directory/x5c.txt').read }
let(:x5c) { Rails.root.join('engines/commercial/id_azure_active_directory/spec/fixtures/x5c.txt').read }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let(:x5c) { Rails.root.join('engines/commercial/id_azure_active_directory/spec/fixtures/x5c.txt').read }
let(:x5c) { IdAzureActiveDirectory::Engine.root.join('spec/fixtures/x5c.txt').read }

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.

3 participants