Adding support for b2b vpp #12420

heartofagoof · 2018-04-30T22:33:55Z

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I've read the Contribution Guidelines
I've updated the documentation if necessary.

Motivation and Context

I wished to provide the support to enable vpp and add the vpp email addresses. I have deliberately not provided a way to remove them or disable vpp because i do not want to introduce changes that may mess up the system.

I have tested adding b2b users when existing users present. also enabled b2b distribution when it was disabled. I have also ensured it by making sure existing spec tests are working - which is a good test in this case since the specs for availability were quite extensive.

Description

Created a class b2b_user for parsing the user details
Provided parsing for b2bAppEnabled, b2bAppFlagDisabled and b2bUsers
Updated TunesClient, AvailabilitySpec, TunesStubbing for this change and added a couple of fixtures for tests.

joshdholtz

This looks good! Just a few small comments 🙃

joshdholtz · 2018-05-01T10:29:57Z

spaceship/lib/spaceship/tunes/availability.rb

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


Can you add a comment above this one?

joshdholtz · 2018-05-01T10:30:26Z

spaceship/lib/spaceship/tunes/availability.rb

+      # Sets the b2b flag. If you call Save on app_details without adding any b2b users
+      # it will result in an error.
+      def enable_b2b_app!
+        print("b2b : " + b2b_unavailable.to_s)


Can you 💣 this print?

joshdholtz · 2018-05-01T10:35:06Z

spaceship/lib/spaceship/tunes/availability.rb

+          b2b_user_array.push(B2bUser.from_username(user))
+        end
+        @b2b_users = b2b_user_array
+        return self


This method looks like its "setting" users instead of "adding" users. Should this method be renamed? Also, this method could be cleaned up to look like the follow if it is actually just "setting"...

def add_b2b_users(user_list = []) raise "Cannot add b2b users if b2b is not enabled" unless b2b_app_enabled @b2b_users = map do |user| B2bUser.from_username(user) end return self end

@joshdholtz its peculiar but when i examined the request body, only the ones that were being added or removed were included in the request body. so this is indeed "adding" instead of "setting". But your comment about "cleanup" stands. I will make the change.

heartofagoof · 2018-05-01T20:45:44Z

@joshdholtz see if this looks good! I m doing some more testing. I ll leave you a comment when i m done.

heartofagoof · 2018-05-01T22:52:05Z

Oops I think I found a bug! Do not merge. I m working on a fix.

heartofagoof · 2018-05-03T19:49:12Z

Fixed. 😥

joshdholtz · 2018-05-04T15:17:56Z

@heartofagoof I will pull this down and test it later today / over this weekend! Would be able to provide me with some code examples I can play with for all the functionalities that you added? 😊 I can replace with all of my app ids and stuff but nice to have an example to base it off of 💪

heartofagoof · 2018-05-04T17:02:45Z

@joshdholtz Something like this should work. I tried it out.

require 'spaceship'  
Spaceship::Tunes.login()
Spaceship::Tunes.select_team 
app = Spaceship::Tunes::Application.find("")
a = app.availability
a = a.enable_b2b_app!
a = a.add_b2b_users([""])
app.update_availability!(a)

And then try to just add b2b users when the b2b is enabled. That should work too.

What should fail is this :

if it is not possible to enable b2b, it should raise an exception
ifit is not enabled when trying to add b2b users it should also raise an exception.

heartofagoof · 2018-05-04T20:21:03Z

Also @joshdholtz , that email id will have to have been enrolled in vpp program - as far as i know.

heartofagoof · 2018-05-07T17:17:13Z

how's it looking @joshdholtz ?

joshdholtz · 2018-05-15T22:25:27Z

spaceship/lib/spaceship/tunes/availability.rb

      )

      # 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.


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?)

joshdholtz · 2018-05-15T22:37:06Z

spaceship/lib/spaceship/tunes/availability.rb

+      end
+
+      # just adds users to the availability, You will still have to call update_availabilty
+      def add_b2b_users(user_list = [])


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

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?

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.

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

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. :)

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:)

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

joshdholtz

Just a few nitpicks on the comments and then I think we will be good 😊

joshdholtz · 2018-05-16T23:32:52Z

spaceship/lib/spaceship/tunes/availability.rb

      def cleared_for_preorder
        super || false
      end
+
+      # Sets the b2b flag. If you call Save on app_details without adding any b2b users


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

joshdholtz · 2018-05-16T23:34:22Z

spaceship/lib/spaceship/tunes/availability.rb

+        self
+      end
+
+      # just adds users to the availability, You will still have to call update_availabilty


And...

# Adds users for b2b enabled apps

joshdholtz · 2018-05-17T00:24:21Z

@snatchev Can you also take a look at this when you get a chance? 😊 I've reviewed and tested it and it looks 💯 but just another set of 👀 would be 💪

joshdholtz

Looks good! 👍 But waiting for one more set of 👀

snatchev

I think we can take advantage of the attr_mapping to get the data back out in the right format when we make the request. This is more of a nice-to-have if it works correctly (which I haven't tested).

Otherwise, it looks good to me. Nice specs :)

snatchev · 2018-05-17T13:47:43Z

spaceship/lib/spaceship/tunes/tunes_client.rb

      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 } } } : []


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.

@heartofagoof Do you mind trying this out? ^ 😊

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.

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

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

fastlane-bot · 2018-05-18T19:30:46Z

Hey @heartofagoof 👋

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 · 2018-05-22T19:32:59Z

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

Adding support for b2b vpp

5030d85

googlebot added the cla: yes label Apr 30, 2018

joshdholtz requested changes May 1, 2018

View reviewed changes

Format and doc changes to address comments

c0d82f0

Fixing a bug with enabling availability

6a9f644

heartofagoof force-pushed the vpp-enablement branch from aa51579 to 6a9f644 Compare May 3, 2018 19:50

fastlane-bot added the status: needs-attention label May 15, 2018

joshdholtz requested changes May 15, 2018

View reviewed changes

Fixing doc

c5b8db3

joshdholtz requested changes May 16, 2018

View reviewed changes

further changes to documentation

05439f4

joshdholtz requested a review from snatchev May 17, 2018 00:23

joshdholtz approved these changes May 17, 2018

View reviewed changes

snatchev approved these changes May 17, 2018

View reviewed changes

joshdholtz merged commit e711a01 into fastlane:master May 17, 2018

fastlane-bot removed the status: needs-attention label May 18, 2018

fastlane-bot added the status: included-in-next-release label May 18, 2018

This was referenced May 21, 2018

Version bump to 2.96.0 #12572

Closed

Version bump to 2.96.0 #12575

Merged

fastlane-bot removed the status: included-in-next-release label May 22, 2018

fastlane-bot added the status: released label May 22, 2018

fastlane locked and limited conversation to collaborators Jul 22, 2018

Adding support for b2b vpp #12420

Adding support for b2b vpp #12420

Conversation

heartofagoof commented Apr 30, 2018 • edited

Checklist

Motivation and Context

Description

joshdholtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heartofagoof commented May 1, 2018 • edited

heartofagoof commented May 1, 2018

heartofagoof commented May 3, 2018

joshdholtz commented May 4, 2018

heartofagoof commented May 4, 2018

heartofagoof commented May 4, 2018

heartofagoof commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdholtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdholtz commented May 17, 2018

joshdholtz left a comment

Choose a reason for hiding this comment

snatchev left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heartofagoof May 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fastlane-bot commented May 18, 2018

fastlane-bot commented May 22, 2018

heartofagoof commented Apr 30, 2018 •

edited

heartofagoof commented May 1, 2018 •

edited

snatchev left a comment •

edited

heartofagoof May 17, 2018 •

edited