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

Adding support for b2b vpp #12420

Merged
merged 5 commits into from
May 17, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 53 additions & 2 deletions spaceship/lib/spaceship/tunes/availability.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require_relative 'territory'

require_relative 'b2b_user'
module Spaceship
module Tunes
class Availability < TunesBase
Expand All @@ -15,15 +15,32 @@ class Availability < TunesBase
# @return (String) App available date in format of "YYYY-MM-DD"
attr_accessor :app_available_date

# @return (Bool) app enabled for b2b users
attr_accessor :b2b_app_enabled

# @return (Bool) app enabled for educational discount
attr_accessor :educational_discount

# @return (Bool) b2b available for distribution
attr_accessor :b2b_unavailable

# @return (Array of Spaceship::Tunes::B2bUser objects) A list of users set by user - if not
# then the b2b user list that is currently set
attr_accessor :b2b_users
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment above this one?


attr_mapping(
'theWorld' => :include_future_territories,
'preOrder.clearedForPreOrder.value' => :cleared_for_preorder,
'preOrder.appAvailableDate.value' => :app_available_date
'preOrder.appAvailableDate.value' => :app_available_date,
'b2BAppFlagDisabled' => :b2b_unavailable
)

# Create a new object based on a set of territories.
# @param territories (Array of String or Spaceship::Tunes::Territory objects): A list of the territories
# @param params (Hash): Optional parameters (include_future_territories (Bool, default: true) Are future territories included?)
# This method has serious implications on vpp + educational discount but i don't want to break
# existing support. please be very careful when using this and provide appropriate params.
# In future, we should create availability from territories as an instance method not as a class method.
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this comment block after the @param and move it to a new line between the Create and the first @param saying something like...

Create a new object based on a set of territories.
This will override any values already set for cleared_for_preorder, app_available_date, b2b_unavailable, b2b_app_enabled, and educational_discount
# @param territories (Array of String or Spaceship::Tunes::Territory objects): A list of the territories
# @param params (Hash): Optional parameters (include_future_territories (Bool, default: true) Are future territories included?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

def self.from_territories(territories = [], params = {})
# Initializes the DataHash with our preOrder structure so values
# that are being modified will be saved
Expand Down Expand Up @@ -51,16 +68,50 @@ def self.from_territories(territories = [], params = {})
obj.include_future_territories = params.fetch(:include_future_territories, true)
obj.cleared_for_preorder = params.fetch(:cleared_for_preorder, false)
obj.app_available_date = params.fetch(:app_available_date, nil)
obj.b2b_unavailable = params.fetch(:b2b_unavailable, false)
obj.b2b_app_enabled = params.fetch(:b2b_app_enabled, false)
obj.educational_discount = params.fetch(:educational_discount, true)
return obj
end

def territories
@territories ||= raw_data['countries'].map { |info| Territory.new(info) }
end

def b2b_users
@b2b_users ||= raw_data['b2bUsers'].map { |user| B2bUser.new(user) }
end

def b2b_app_enabled
@b2b_app_enabled.nil? ? raw_data['b2bAppEnabled'] : @b2b_app_enabled
end

def educational_discount
@educational_discount.nil? ? raw_data['educationalDiscount'] : @educational_discount
end

def cleared_for_preorder
super || false
end

# Sets the b2b flag. If you call Save on app_details without adding any b2b users
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this comment to...

# Sets `b2b_app_enabled` to true and `educational_discount` to false
# Requires users to be added with `add_b2b_users` otherwise `update_availability` will error

# it will result in an error.
def enable_b2b_app!
raise "Not possible to enable b2b on this app" if b2b_unavailable
@b2b_app_enabled = true
# need to set the educational discount to false
@educational_discount = false
self
end

# just adds users to the availability, You will still have to call update_availabilty
Copy link
Member

Choose a reason for hiding this comment

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

And...

# Adds users for b2b enabled apps

def add_b2b_users(user_list = [])
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually "set" the users and not "add" users? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If this does set/override the @b2b_users, could we maybe combine enable_b2b_app! with this one and just pass the user list into the enable_b2_app! method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not set or override. It may look like this but the way the requests are ( if you observe the object ) only the email ids either being added or removed are sent over.

Copy link
Member

Choose a reason for hiding this comment

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

@heartofagoof Interesting 🤔 Good to know! So this could also remove the user if the user was already set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. For that you'd have to set removed = true. Which we are not setting right now. I didn't want to implement anything destructive. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can of course change the implementation to something like this if it helps :

update_b2b_users(users_to_add: [], users_to_remove: [])

and update the signature to:

from_username(email_id: [], add_or_remove:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way we provide the ability to add and remove b2b teams.

raise "Cannot add b2b users if b2b is not enabled" unless b2b_app_enabled
@b2b_users = user_list.map do |user|
B2bUser.from_username(user)
end
self
end
end
end
end
25 changes: 25 additions & 0 deletions spaceship/lib/spaceship/tunes/b2b_user.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
require_relative 'tunes_base'
module Spaceship
module Tunes
class B2bUser < TunesBase
# @return (Bool) add the user to b2b list
attr_accessor :add

# @return (Bool) delete the user to b2b list
attr_accessor :delete

# @return (String) b2b username
attr_accessor :ds_username

attr_mapping(
'value.add' => :add,
'value.delete' => :delete,
'value.dsUsername' => :ds_username
)

def self.from_username(username)
self.new({ 'value' => { 'add' => true, 'delete' => false, 'dsUsername' => username } })
end
end
end
end
5 changes: 3 additions & 2 deletions spaceship/lib/spaceship/tunes/tunes_client.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
require "securerandom"

require_relative '../client'
require_relative '../du/du_client'
require_relative '../du/upload_file'
Expand All @@ -11,7 +10,6 @@
require_relative 'pricing_tier'
require_relative 'territory'
require_relative 'user_detail'

module Spaceship
# rubocop:disable Metrics/ClassLength
class TunesClient < Spaceship::Client
Expand Down Expand Up @@ -613,8 +611,11 @@ def update_availability!(app_id, availability)
# API will error out if cleared_for_preorder is false and app_available_date has a date
cleared_for_preorder = availability.cleared_for_preorder
app_available_date = cleared_for_preorder ? availability.app_available_date : nil
data["b2bAppEnabled"] = availability.b2b_app_enabled
data["educationalDiscount"] = availability.educational_discount
data["preOrder"]["clearedForPreOrder"] = { "value" => cleared_for_preorder, "isEditable" => true, "isRequired" => true, "errorKeys" => nil }
data["preOrder"]["appAvailableDate"] = { "value" => app_available_date, "isEditable" => true, "isRequired" => true, "errorKeys" => nil }
data["b2bUsers"] = availability.b2b_app_enabled ? availability.b2b_users.map { |user| { "value" => { "add" => user.add, "delete" => user.delete, "dsUsername" => user.ds_username } } } : []
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can convert the b2b_users back into data by doing: b2b_users.raw_data.to_h. Ideally, the model and the attr_mapping should be the only place that has to know how to convert to and from the JSON data.

Copy link
Member

Choose a reason for hiding this comment

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

@heartofagoof Do you mind trying this out? ^ 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay digging into the metaprogramming ya'll have to make sense of it. I tried to use the mapping but it was not working quite as expected - it might be the limitation of the model.

I am more than happy to give it a proper go, however. If it is limited in scope, I can make the changes here but if it is extensive, hopefully I can make it in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @heartofagoof, if it doesn't work as expected out of the box, I wouldn't bother too much trying to hack it to work

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, no worries at all! We will merge what you got here 😊


# send the changes back to Apple
r = request(:post) do |req|
Expand Down
52 changes: 52 additions & 0 deletions spaceship/spec/tunes/availability_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@
expect(availability.include_future_territories).to be_truthy
end

it "correctly parses b2b app enabled" do
availability = client.availability(app.apple_id)

expect(availability.b2b_app_enabled).to be(false)
end

it "correctly parses b2b app flag enabled" do
availability = client.availability(app.apple_id)

expect(availability.b2b_unavailable).to be(false)
end

it "correctly parses the territories" do
availability = client.availability(app.apple_id)

Expand All @@ -31,6 +43,15 @@
expect(territory_0.region).to eq('Europe')
expect(territory_0.region_locale_key).to eq('ITC.region.EUR')
end

it "correctly parses b2b users" do
TunesStubbing.itc_stub_app_pricing_intervals_vpp
availability = client.availability(app.apple_id)
expect(availability.b2b_users.length).to eq(2)
b2b_user_0 = availability.b2b_users[0]
expect(b2b_user_0).not_to(be_nil)
expect(b2b_user_0.ds_username).to eq('b2b1@abc.com')
end
end

describe "update_availability!" do
Expand Down Expand Up @@ -135,5 +156,36 @@

expect(availability.include_future_territories).to be_falsey
end

describe "enable_b2b_app!" do
it "throws exception if b2b cannot be enabled" do
TunesStubbing.itc_stub_app_pricing_intervals_b2b_disabled
availability = client.availability(app.apple_id)
expect { availability.enable_b2b_app! }.to raise_error("Not possible to enable b2b on this app")
end

it "works correctly" do
availability = client.availability(app.apple_id)
new_availability = availability.enable_b2b_app!
expect(new_availability.b2b_app_enabled).to eq(true)
expect(new_availability.educational_discount).to eq(false)
end
end

describe "add_b2b_users" do
it "throws exception if b2b app not enabled" do
availability = client.availability(app.apple_id)
expect { availability.add_b2b_users(["abc@def.com"]) }.to raise_error("Cannot add b2b users if b2b is not enabled")
end

it "works correctly" do
TunesStubbing.itc_stub_app_pricing_intervals_vpp
availability = client.availability(app.apple_id)
new_availability = availability.add_b2b_users(["abc@def.com"])
expect(new_availability).to be_an_instance_of(Spaceship::Tunes::Availability)
expect(new_availability.b2b_users.length).to eq(1)
expect(new_availability.b2b_users.first.ds_username).to eq("abc@def.com")
end
end
end
end
45 changes: 45 additions & 0 deletions spaceship/spec/tunes/b2b_user_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
require 'spec_helper'
class B2bUserSpec
describe Spaceship::Tunes::B2bUser do
before { Spaceship::Tunes.login }
before { TunesStubbing.itc_stub_app_pricing_intervals }

let(:client) { Spaceship::AppVersion.client }
let(:app) { Spaceship::Application.all.first }
let(:mock_client) { double('MockClient') }
let(:b2b_user) do
Spaceship::Tunes::B2bUser.new(
'value' => {
'dsUsername' => "b2b1@abc.com",
'delete' => false,
'add' => false,
'company' => 'b2b1'
},
"isEditable" => true,
"isRequired" => false
)
end
before do
allow(Spaceship::Tunes::TunesBase).to receive(:client).and_return(mock_client)
allow(mock_client).to receive(:team_id).and_return('')
end

describe 'b2b_user' do
it 'parses the data correctly' do
expect(b2b_user).to be_instance_of(Spaceship::Tunes::B2bUser)
!expect(b2b_user.add)
!expect(b2b_user.delete)
expect(b2b_user.ds_username).to eq('b2b1@abc.com')
end
end

describe 'from_username' do
it 'creates correct object to add' do
b2b_user_created = Spaceship::Tunes::B2bUser.from_username("b2b2@def.com")
expect(b2b_user_created.add)
!expect(b2b_user_created.delete)
expect(b2b_user_created.ds_username).to eq('b2b2@def.com')
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
{
"data": {
"sectionErrorKeys": [

],
"sectionInfoKeys": [

],
"sectionWarningKeys": [

],
"countriesChanged": false,
"countries": [
{
"code": "GB",
"name": "United Kingdom",
"region": "Europe",
"regionLocaleKey": "ITC.region.EUR",
"currencyCodeISO3A": "GBP"
},
{
"code": "US",
"name": "United States",
"region": "The United States and Canada",
"regionLocaleKey": "ITC.region.NAM",
"currencyCodeISO3A": "USD"
}
],
"b2BAppFlagDisabled": true,
"educationDiscountDisabledFlag": false,
"hotFudgeFeatureEnabled": true,
"coldFudgeFeatureEnabled": false,
"bitcodeAutoRecompileDisAllowed": false,
"b2bAppEnabled": false,
"educationalDiscount": true,
"theWorld": true,
"pricingIntervalsFieldTO": {
"value": [
{
"priceTierEffectiveDate": null,
"priceTierEndDate": null,
"tierStem": "0"
}
],
"isEditable": true,
"isRequired": false,
"errorKeys": null
},
"availableDate": 1476687600000,
"unavailableDate": -2,
"creatingApp": false,
"hasApprovedVersion": false,
"preOrder":{
"clearedForPreOrder":{
"value":false,
"isEditable":true,
"isRequired":true,
"errorKeys":null
},
"appAvailableDate":{
"value":null,
"isEditable":true,
"isRequired":true,
"errorKeys":null
},
"preOrderAvailableDate":null
},
"appVersionsForOTAByPlatforms": {
"iOS": [

]
},
"b2bUsers": [

]
},
"messages": {
"warn": null,
"info": null,
"error": null
},
"statusCode": "SUCCESS"
}