Ms/user access management

[msokoloff1]

PLAT-1224

The purpose of this pr is to allow customers to manage organization invites and user permissions from the SDK. The following functionality was added:

• create / update / revoke invite
• query for active invites
• query for remaining allowed invites to an organization
• query for remaining available seats in an organization
• set and update organization roles
• assign users to projects
  • set / update / revoke project role
• delete users from org
• Example notebook

[nmaswood]

Going to take a second look tommorow when I am fresh!

I might ask Greg to also take a look, because I think experimental mode is a big change and we should be cautious in how we roll it out in terms of the API

also I think Grant/Collin might be helpful in providing some context of how the invite api ought to be used.

[msokoloff1]

After speaking with Grant, we decided to remove the ability to query and revoke active invites since this might behave unexpectedly. He also said we need to remove the old user_limit query in favor of the new invite_limit query. All of these requested changes have been made.

[nmaswood]

will continue after stand up

[gszpak]

Still wrapping my head around all this, but are we sure we want to raise an error here? What if we want to do lazy loading of cached relationships?

[msokoloff1]

If we need that in the future we can make the change. I think making this more strict is better.

[gszpak]

In general I'm not a huge fan of overriding __new__. Imho the code gets overly complicated. If we want to make them singletons, could we instead declare the roles as lazy initalized module vars - sth like here https://github.com/Labelbox/python-monorepo/blob/6c6bfebb68fc6f876c3abf29c9ce727a6c5744dc/services/prediction_import/prediction_import/db/client.py#L357

[msokoloff1]

This is a pretty standard singleton pattern. The code snippet you provided is much simpler but also because it is doing much less. This is going to basically look the same except swap new for init and add a new module level variable which feels like a lateral move rather than an improvement.

[nmaswood]

I lean more towards, structs/class should be dumb containers of data than locations of business logic.

In that sense I think you could seperate fetching GetAvailableUserRolesPyApi logic from the class __new__ and move it out into a static method or function, imho it doesn make reading the class a little trickier.

class RoleAccessor(self):
    def __init__(self):
         self.roles = get_The_roles(client)
    

or some other means to break up the logic

[msokoloff1]

I changed it to this for now.

labelbox-python/labelbox/schema/role.py

Lines 8 to 52 in e6e4dd6

	def _get_roles(client):
	query_str = """query GetAvailableUserRolesPyApi { roles { id name } }"""
	if not hasattr(_get_roles, 'roles'):
	_get_roles.roles = res = client.execute(query_str)
	return _get_roles.roles
	class Roles:
	"""
	Object that manages org and user roles
	>>> roles = client.get_roles()
	>>> roles.valid_roles # lists all valid roles
	>>> roles['ADMIN'] # returns the admin Role
	"""
	def __init__(self, client):
	res = _get_roles(client)
	valid_roles = set()
	for result in res['roles']:
	_name = result['name'].upper().replace(' ', '_')
	result['name'] = _name
	setattr(self, _name, Role(client, result))
	valid_roles.add(_name)
	self.valid_roles = valid_roles
	def __repr__(self):
	return str({k: getattr(self, k) for k in self.valid_roles})
	def __getitem__(self, name):
	name = name.replace(' ', '_').upper()
	if name not in self.valid_roles:
	raise ValueError(
	f"No role named {name} exists. Valid names are one of {self.valid_roles}"
	)
	return getattr(self, name)
	def __iter__(self):
	self.key_iter = iter(self.valid_roles)
	return self
	def __next__(self):
	key = next(self.key_iter)
	return getattr(self, key)

. I'm not totally clear what Nasr is suggesting so we are going to sync tomorrow.

[nmaswood]

I lean more towards, structs/class should be dumb containers of data than locations of business logic.

In that sense I think you could seperate fetching GetAvailableUserRolesPyApi logic from the class __new__ and move it out into a static method or function, imho it doesn make reading the class a little trickier.

class RoleAccessor(self):
    def __init__(self):
         self.roles = get_The_roles(client)
    

or some other means to break up the logic

[nmaswood]

You could maybe turn this comments into application logic through introducing a dummy decorator that annotates like test method only or something like that.

or like throw if pytest is not running:

https://stackoverflow.com/questions/25188119/test-if-code-is-executed-from-within-a-py-test-session

[msokoloff1]

The main point of the comment is to make it so that users don't copy this into their own application. I don't think there is a risk of someone importing from the conftest outside of the tests.

[nmaswood]

is this gonna make int. tests flakier?

[msokoloff1]

No it clears invites before and after (only @labelbox.com invites so users don't accidentally mess up anything). Our test account is only used for testing so there shouldn't be any problem here. This will only matter if you are testing locally and don't have any open seats.

[nmaswood]

This feels really hard-coded in a way. I would expect the basic execute function to maybe catch top level errors, but not super specific errors from particular queries.

also InvalidQueryError to me implies that the query was malformed as opposed to something about their user limits

[msokoloff1]

This is because the backend just throws a 500 and there is no way to handle this without parsing the message (see the comment just above this.

Server side validation is one reason we raise this exception:

labelbox-python/labelbox/exceptions.py

Lines 63 to 65 in 56a366c

	""" Indicates a malconstructed or unsupported query (either by GraphQL in
	general or by Labelbox specifically). This can be the result of either client
	or server side query validation. """

I don't like this pattern either but I don't think fixing this is within the scope of this delivery.

[nmaswood]

aite, one small nit is that startswith takes a tuple I believe
message.startsiwh(("message1, "message"))

[msokoloff1]

Fixed.

[nmaswood]

Is adding TYPE_CHECKING check standard?

Do we add it for compatibility or performance reasons?

[msokoloff1]

It is to avoid circular imports. There is not really a great solution here.

[nmaswood]

jw, do we even need to format_roles now that we are not emulating enum behaavior?

[msokoloff1]

Idk I don't like spaces and inconsistent casing in key names. It makes it harder to use.

[nmaswood]

@gszpak it all looks good to me, you have any follow up thoughts?