Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion app/models/invitation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class Invitation < ApplicationRecord

# Build new user on information in this invitation.
def make_user(params = {})
self.user = account.users.build_with_fields params.reverse_merge(email: email, invitation: self)
self.user = account.users.build_with_fields params.reverse_merge(email: email)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand if we want to remove this method. But as it is, I think it makes more sense to stay as it was. With the change, I don't think the association will be present in the object.

end

def accepted?
Expand Down
8 changes: 3 additions & 5 deletions app/models/user/invitations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ module User::Invitations
included do
after_commit :accept_invitation, :on => :create

attr_accessor :invitation

# TODO: refactor to make this work removing above attribute.
# has_one :invitation
has_one :invitation # rubocop:disable Rails/HasManyOrHasOneDependent -- before_destroy :destroy_invitation handles it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this will also work without rubocop exclusions

Suggested change
has_one :invitation # rubocop:disable Rails/HasManyOrHasOneDependent -- before_destroy :destroy_invitation handles it
has_one :invitation, dependent: nil

This will communicate intent without ugliness, also I hope rubocop will understand this was intentional and not complain as well


before_destroy :destroy_invitation
end
Expand All @@ -18,7 +15,8 @@ def accept_invitation

def destroy_invitation
if account # some tests fail because of account being nil
invit = account.invitations.find_by_email(email) || account.invitations.find_by_user_id(id) # || eventually invitation
invitations = account.invitations
invit = invitations.find_by(email: email) || invitations.find_by(user_id: id)
# halt the destruction if the destruction of invitation failed
throw :abort if invit && invit.destroy == false
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def setup

assert_equal I18n.t('developer_portal.accounts.invitee_signups.create.success'), flash[:notice]
assert_redirected_to login_path
assert invitation.reload.accepted?
end

test 'create pushes webhook' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def setup

assert_equal I18n.t('provider.invitee_signups.create.success'), flash[:success]
assert_redirected_to provider_login_path
assert invitation.reload.accepted?
end

test 'do not set unpermitted attributes' do
Expand Down
1 change: 1 addition & 0 deletions test/unit/authentication/strategy/oauth2_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ class SsoSignupTest < ActiveSupport::TestCase
assert_equal result.username, user_data[:username]
assert result.active?
assert authentication_strategy.error_message.blank?
assert invitation.reload.accepted?
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Found 18 lines of similar code in 2 locations (mass = 93) [qlty:similar-code]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna ignore these, I don't see the problem on the duplication, these are tests.

end
end
Expand Down
1 change: 1 addition & 0 deletions test/unit/authentication/strategy/provider_oauth2_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ class SsoSignupTest < ActiveSupport::TestCase
assert_equal result.username, user_data[:username]
assert result.active?
assert authentication_strategy.error_message.blank?
assert invitation.reload.accepted?
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Found 18 lines of similar code in 2 locations (mass = 93) [qlty:similar-code]

end
end
Expand Down