[spaceship] Add visible_apps relationship to invite users with app permissions and fetch user's app permissions #19053

lucgrabowski · 2021-07-04T21:18:15Z

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

User and UserInvitation models should have a relationship to visible_apps to manage app permissions.

Description

Changes:
It's possible to assign app permissions when inviting user with post_user_invitation.
It's possible to fetch app permissions (visible_apps) for both User and UserInvitation

Testing Steps

Testing in irb:

require 'spaceship'
Spaceship::ConnectAPI.login

users = Spaceship::ConnectAPI::User.all(includes: Spaceship::ConnectAPI::User::ESSENTIAL_INCLUDES)
# or
users = Spaceship::ConnectAPI::User.all(includes: "visibleApps")
# or
users = Spaceship::ConnectAPI::User.all

user = Spaceship::ConnectAPI::User.find(email:"USER_EMAIL").first
user.get_visible_apps 
user.get_visible_apps(limit: 1)

user = Spaceship::ConnectAPI::User.find(email:"USER_EMAIL", includes: "visibleApps")


invitations = Spaceship::ConnectAPI::UserInvitation.all(includes: Spaceship::ConnectAPI::UserInvitation::ESSENTIAL_INCLUDES)
# or
invitations = Spaceship::ConnectAPI::UserInvitation.all(includes: "visibleApps")
# or 
invitations = Spaceship::ConnectAPI::UserInvitation.all

Spaceship::ConnectAPI::UserInvitation.create(email: "YOUR_EMAIL", first_name: "YOUR_FIRST_NAME", last_name: "YOUR_LAST_NAME", roles: ["DEVELOPER"], provisioning_allowed: true, all_apps_visible: false, visible_app_ids: ["APP_ID"])
user_invitation = Spaceship::ConnectAPI::UserInvitation.find("YOUR_EMAIL")
user_invitation.get_visible_apps
user_invitation.get_visible_apps(limit: 1)
user_invitation.first.delete!

# Testing of invalid combinations of `all_apps_visible` and `visible_app_ids`:

Spaceship::ConnectAPI::UserInvitation.create(email: "YOUR_EMAIL", first_name: "YOUR_FIRST_NAME", last_name: "YOUR_LAST_NAME", roles: ["DEVELOPER"], provisioning_allowed: true, all_apps_visible: true, visible_app_ids: ["APP_ID"])
# It will fail with "The provided entity includes a relationship with an invalid value - If you set allAppsVisible to true, you must not provide values for the visibleApps relationship. - /data/relationships/visibleApps"

Spaceship::ConnectAPI::UserInvitation.create(email: "YOUR_EMAIL", first_name: "YOUR_FIRST_NAME", last_name: "YOUR_LAST_NAME", roles: ["DEVELOPER"], provisioning_allowed: true, all_apps_visible: false)
# It will fail with "The provided visible app information is invalid. - If you set allAppsVisible to false, you must provide at least one value for the visibleApps relationship. - /data/attributes/allAppsVisible

…r invitation

ainame

Thank you for your contribution! I wanted to point how ESSENTIAL_INCLUDES convention is supposed to work🙂 otherwise this looks great change 😍

ainame · 2021-07-10T05:17:03Z

spaceship/lib/spaceship/connect_api/models/user.rb

+      ESSENTIAL_INCLUDES = [
+        "visibleApps"
+      ].join(",")


This isn't automatically used at all. You need to pass this to the parent API calls.

For example, you need to pass it to the corresponding methods as the default value of includes argument. This way App Store Connect API will include "visibleApps" objects within the response and spaceship client will automatically map it to visible_apps property. That's what ESSENTIAL_INCLUDES convention is supposed to work.

def self.all(filter: {}, includes: Spaceship::ConnectAPI::User::ESSENTIAL_INCLUDES, limit: nil, sort: nil)

https://github.com/fastlane/fastlane/pull/19053/files#diff-75baf27580ebd9af6509c2bbdb9fca7cd67b716bf144964ffc8e208c397d57bcR49

https://github.com/fastlane/fastlane/pull/19053/files#diff-75baf27580ebd9af6509c2bbdb9fca7cd67b716bf144964ffc8e208c397d57bcR55

fastlane/spaceship/lib/spaceship/connect_api/models/user.rb

Lines 41 to 45 in c0b94f1

def self.all(client: nil, filter: {}, includes: nil, limit: nil, sort: nil)

client ||= Spaceship::ConnectAPI

resps = client.get_users(filter: filter, includes: includes).all_pages

return resps.flat_map(&:to_models)

end

Otherwise visible_apps property ends up returning nil depending on how you get the User object.

Thanks for the explanatory comment @ainame.
I compared how ESSENTIAL_INCLUDES are used in App, AppStoreVersion models and I thought for a while if user visible_apps should be fetched by default and I wasn't sure if I should use them as default inlcudes argument.
I added the definition though to allow to use it as in example testing steps, e.g.

users = Spaceship::ConnectAPI::User.all(includes: Spaceship::ConnectAPI::User::ESSENTIAL_INCLUDES)

Otherwise those that would like to fetch it would have to investigate what value should be used in includes.

Should visible_apps be fetched by default in self.find and self.all methods?

My point is that if it's not "by default", it shouldn't be named "ESSENTIAL". It sounds more like OPTIONAL_INCLUDES to me.
https://developer.apple.com/documentation/appstoreconnectapi/list_users

Should visible_apps be fetched by default in self.find and self.all methods?

To me, it can be an "essential" info when it comes to handling users for ASC. Any advice to determine if it's essential? @joshdholtz

If the advice will be to go with optional, I'll rename it to OPTIONAL_INCLUDES. Otherwise, I'll add essential as default values for includes parameters in self.all and self.find.

Wow, I missed this conversation completely 😱 Sorry! I had my notifications under control for a while but lost control this past month 😬 But...

I think essential is fine here!

ainame · 2021-07-10T05:25:37Z

spaceship/lib/spaceship/connect_api/models/user_invitation.rb

      })

+      ESSENTIAL_INCLUDES = [


The same applies to this. You need to pass this to self.all and self.find.

ainame · 2021-07-10T05:33:07Z

spaceship/lib/spaceship/connect_api/models/user.rb

@@ -48,6 +56,12 @@ def self.find(client: nil, email: nil, includes: nil)
        client ||= Spaceship::ConnectAPI
        return all(client: client, filter: { email: email }, includes: includes)
      end
+
+      def fetch_visible_apps(client: nil, limit: nil)


@joshdholtz Any thought on naming this kind of method with get vs fetch? 🤔

@ainame This also made me think for a while and compare code in other models and I wasn't sure if the choice is correct. 🤔

Me too honestly. Some uses "get" others do "fetch" 😅

UGGHHHH... I hate naming 😅 I honestly don't know which it should be but I do want it to be consistent at some point 🤷‍♂️

I've renamed it to get and it's used more in code.

…nvitation

joshdholtz

Tested this out and its working good 😊 Thanks for adding this! Really appreciate the contribution ❤️

fastlane-bot

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

lucgrabowski added 14 commits July 4, 2021 09:37

Add visibleApps to User and UserInvitation

06aaaf5

Use visible_apps in create UserInvitation

ad7ecc1

Use visible_apps in post_user_invitation

c648251

Get visible apps for invited user

0be1cd8

Use visible_apps in post_user_invitation - fix to use relationships

da55189

Rename visible_apps param to visible_app_ids

26956f2

Explain how to use all_apps_visible and visible_app_ids to create use…

a9eb2c2

…r invitation

Fetch visible apps response fix

f2d5619

Use all_pages in fetch_visible_apps

eacf3a3

Fetch visible apps for user

1239393

Add tests for get requests, build_params for calls

1283815

Add limit param for get requests

690d9fa

Add test for post_user_invitation

28eca2e

Rubocop fixes

3900bdb

google-cla bot added the cla: yes label Jul 4, 2021

Add limit param to fetch_visible_apps methods

02a2c9d

ainame requested changes Jul 10, 2021

View reviewed changes

fastlane-bot added the status: needs-attention label Jul 19, 2021

lucgrabowski added 2 commits August 1, 2021 16:27

Code review improvements - rename fetch to get

9f55ad7

Code review improvements - get essential includes for user and user_i…

3586dc8

…nvitation

lucgrabowski requested review from ainame and joshdholtz August 2, 2021 06:12

joshdholtz approved these changes Aug 2, 2021

View reviewed changes

joshdholtz merged commit 5fb3ff7 into fastlane:master Aug 2, 2021

joshdholtz mentioned this pull request Aug 2, 2021

Version bump to 2.190.0 #19193

Merged

fastlane-bot added the status: released label Aug 3, 2021

fastlane-bot reviewed Aug 3, 2021

View reviewed changes

fastlane-bot removed the status: needs-attention label Aug 9, 2021

ohthepain mentioned this pull request Aug 18, 2021

Fastlane keeps reverting to v 2.184.1 #19245

Closed

4 tasks

fastlane locked and limited conversation to collaborators Oct 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[spaceship] Add visible_apps relationship to invite users with app permissions and fetch user's app permissions #19053

[spaceship] Add visible_apps relationship to invite users with app permissions and fetch user's app permissions #19053

lucgrabowski commented Jul 4, 2021 •

edited

Loading

ainame left a comment

ainame Jul 10, 2021

ainame Jul 10, 2021

lucgrabowski Jul 10, 2021

ainame Jul 11, 2021

lucgrabowski Jul 11, 2021

joshdholtz Aug 2, 2021

ainame Jul 10, 2021

ainame Jul 10, 2021

lucgrabowski Jul 10, 2021

ainame Jul 11, 2021

joshdholtz Jul 27, 2021

lucgrabowski Aug 1, 2021

joshdholtz left a comment

fastlane-bot left a comment

	def self.all(client: nil, filter: {}, includes: nil, limit: nil, sort: nil)
	client \|\|= Spaceship::ConnectAPI
	resps = client.get_users(filter: filter, includes: includes).all_pages
	return resps.flat_map(&:to_models)
	end

[spaceship] Add visible_apps relationship to invite users with app permissions and fetch user's app permissions #19053

[spaceship] Add visible_apps relationship to invite users with app permissions and fetch user's app permissions #19053

Conversation

lucgrabowski commented Jul 4, 2021 • edited Loading

Checklist

Motivation and Context

Description

Testing Steps

ainame 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

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

fastlane-bot left a comment

Choose a reason for hiding this comment

lucgrabowski commented Jul 4, 2021 •

edited

Loading