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

added team_name to spaceship client #12457

Merged
merged 6 commits into from May 21, 2018

Conversation

RishabhTayal
Copy link
Contributor

Currently the spaceship client doesn't contain a function to set the team_name from iTC. There is a select_team function. However I think it would be better if there is a team_name function similar to team_id to set the name.

@RishabhTayal
Copy link
Contributor Author

@joshdholtz Please take a look whenever you get a chance.

end

# Set a new team Name which will be used from now on
def team_name=(team_name)
Copy link
Member

Choose a reason for hiding this comment

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

I totes see the use case for the team_name but do we really need team_name=? Selected with a team name should be possible with existing functionality in select_team(team_id: nil, team_name: nil), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshdholtz I don't think we really need team_name=. It's a nice to have since we have team_id=. We could definitely use select_team() function to set the team_name.

Feel free to reject this PR. 😄

If you think about it we don't even need team_id= as well. If not, I can remove that code. LMK.

Copy link
Member

Choose a reason for hiding this comment

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

@RishabhTayal We totally need team_id= still for backwards compatibility which is why I didn't recommend deleting that 🙃 But if we could 💣 that team_name= I think this PR is valid to add a team_name attribute/method

puts("The current user is in #{teams.count} teams. Pass a team Name or call `select_team` to choose a team. Using the first one for now.")
end
@current_team_name ||= teams[0]['contentProvider']['name']
end
Copy link
Member

Choose a reason for hiding this comment

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

@RishabhTayal Now that I think about this, we should probably 💣 @current_team_name and replace it as a method that looks up from teams based on @current_team_id

@RishabhTayal
Copy link
Contributor Author

@joshdholtz it's ready to review again.

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Can we add some tests for this? 🙃

@RishabhTayal
Copy link
Contributor Author

Not sure what kind of tests we can add for this. I don't see any tests written for team_id as well.

@joshdholtz
Copy link
Member

@RishabhTayal
Copy link
Contributor Author

@joshdholtz I am not sure this is applicable here. The test you pointed out is for portal_client.rb class. When trying to write similar tests I am getting nil values.

@RishabhTayal
Copy link
Contributor Author

@joshdholtz I have added the tests, however it's failing now. Could you help me get those tests passed?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@joshdholtz
Copy link
Member

@RishabhTayal Updated ^

@RishabhTayal
Copy link
Contributor Author

@joshdholtz could you sign the CLA? 😝🤣🤣

@RishabhTayal
Copy link
Contributor Author

This is ready to review cc @taquitos @revolter

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

💪 🚀

@joshdholtz joshdholtz merged commit 762e13c into fastlane:master May 21, 2018
@RishabhTayal RishabhTayal deleted the spaceship-team-name branch May 21, 2018 15:27
@fastlane-bot
Copy link

Hey @RishabhTayal 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.96.0 🚀

@fastlane fastlane locked and limited conversation to collaborators Jul 22, 2018
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.

None yet

4 participants