Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Fix crash in xcode manager if no user is provided #919

Merged
merged 1 commit into from
Jun 15, 2018
Merged

Conversation

KrauseFx
Copy link
Member

No description provided.

Copy link
Member

@snatchev snatchev left a comment

Choose a reason for hiding this comment

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

While the code changes look good to me, I have a comment on the purpose of this method.

I would recommend instead of having a state-changing behavior inside this method, I think it would be preferable to return the apple_id or raise an error and set @apple_id in the initializer.

@taquitos
Copy link
Collaborator

taquitos commented Jun 7, 2018

@snatchev that's a great point, setting things at initialization and not mutating is 🔑

@KrauseFx
Copy link
Member Author

Okay, we have the XcodeManagerService in our Services class in fastlane.ci. Do you propose we create a new service every time?

Right now, we only set the value during init time, as we don't have a UI/API yet to set a default Apple ID, but in the future this will be possible. Either way is fine for me. I'd propose to go ahead and merge this PR assuming this is still a problem, and create an issue to discuss this further

@snatchev
Copy link
Member

snatchev commented Jun 15, 2018

I think I might not have explained my initial suggestion. Basically, I propose we have a method that returns the apple_id, like this:

def apple_id(user:)
  apple_id = Services.apple_id_service.apple_ids.find { |a| a.user == user } if user
  if apple_id.nil?
    # Think about the user flow on how this would be shown, we probably want to check
    # for the existance of the Apple ID earlier, and then not even show the button,
    # or make the button redirect to the Apple ID login instead
    raise "No registered Apple ID found with user #{user}, make sure to add your Apple account to fastlane.ci"
  end
  return apple_id
end

and we set @apple_id = apple_id(user: user) in the initializer. The benefit being that instance state is set in 1 place in the initializer which is a little more clear, IMO.

I also didn't see any calls to use_apple_id with an apple_id: parameter. If that's unused, we should remove it.

Copy link
Member

@snatchev snatchev left a comment

Choose a reason for hiding this comment

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

I totally mis-understood the purpose of this method, and my feedback is non-applicable.

LGTM!

@KrauseFx
Copy link
Member Author

Alright, would hit the merge button now, but @taquitos would tap on shoulder and slap on finger, please merge as soon as CI is ready again

@taquitos taquitos merged commit d89717d into master Jun 15, 2018
@taquitos taquitos deleted the KrauseFx-patch-1 branch June 15, 2018 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants