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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

produce: fix data_protection in enabled features #7955

Merged
merged 7 commits into from
Jan 24, 2017

Conversation

hjanuschka
Copy link
Collaborator

@hjanuschka hjanuschka commented Jan 23, 2017

fixes: #7954

.on is nil on that option! 馃槩

Copy link
Contributor

@mfurtak mfurtak left a comment

Choose a reason for hiding this comment

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

Looks good. 馃憤

Can you also add a test that covers this code to prevent regressions?

up
@hjanuschka
Copy link
Collaborator Author

@mfurtak good idea, however produce does not have yet any specs.
i added some, but i am failing to suceed, can you have a look? (commented inline)

mgrebenets
mgrebenets previously approved these changes Jan 23, 2017
stub_request(:post, "https://idmsa.apple.com/appleauth/auth/signin?widgetKey=e0b80c3bf78523bfe80974d320935bfa30add02e1bff88ec2166c6bd5a706c42").
with(body: "{\"accountName\":\"helmut@xxx.com\",\"password\":\"xxxx\",\"rememberMe\":true}",
headers: { 'Accept' => 'application/json, text/javascript', 'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type' => 'application/json', 'User-Agent' => 'Spaceship 2.9.0', 'X-Requested-With' => 'XMLHttpRequest' }).
to_return(status: 200, body: "", headers: {})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

stub will be replaced, once the create_new_app is called.

# this does not get triggerd - HELP
# i would return true here, and check if the `enabled_features` argument to create_new_app is the expected one
# but it just does not get called.
expect(Produce::DeveloperCenter).to receive(:create_new_app).with(anything).and_return(false)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see here 馃槩

@hjanuschka hjanuschka dismissed mgrebenets鈥檚 stale review January 23, 2017 12:49

sry - code is not ready

@hjanuschka
Copy link
Collaborator Author

@mgrebenets , @mfurtak all good now - tests pass 馃憤

username: "helmut@januschka.com",
enabled_features: { data_protection: "complete" },
skip_itc: true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

While this works, it's a little sketchy since the real type of this object should be Configuration. It's more common to set this up like:

Produce.config = FastlaneCore::Configuration.create(Produce::Options.available_options, {
  username: "helmut@januschka.com",
  enabled_features: { data_protection: "complete" },
  skip_itc: true
})

@@ -0,0 +1 @@
require_relative "stubbing"
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove this and the stubbing.rb until we need them 馃憤

@hjanuschka
Copy link
Collaborator Author

@mfurtak feedback addressed.

Copy link
Contributor

@mfurtak mfurtak left a comment

Choose a reason for hiding this comment

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

Good stuff!

@hjanuschka hjanuschka merged commit 0842069 into fastlane:master Jan 24, 2017
@KrauseFx KrauseFx mentioned this pull request Jan 24, 2017
@fastlane fastlane locked and limited conversation to collaborators Apr 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

produce enable_services not actually enabling
3 participants