Skip to content

Conversation

@Tim-Kerr
Copy link
Contributor

@Tim-Kerr Tim-Kerr commented Feb 25, 2023

How to test:

from labelbox import Client

client = Client(
    api_key=<LOCALHOST_API_KEY>,
    rest_endpoint="http://localhost:3000/api/api/v1")

res = client.unarchive_feature_schema_node(<ontology_id>, <feature_schema_id>)
print(res)

Given a valid API key that owns the valid ontology and root feature schema node expect True

  • Important: Test unauthorized access *

Use a valid api key for a user in another org. Attempt to unarchive the root feature schema node belonging to another org. Expect to get a "404" response.

https://www.loom.com/share/379eaebf4f5c483fa0fc0ca6656475d9

Returns:
bool
"""
ontology_endpoint = self.rest_endpoint + "/ontologies/" + ontology_id + '/feature-schemas/' + root_feature_schema_id + '/unarchive'
Copy link
Contributor

Choose a reason for hiding this comment

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

please encode ontology_id and root_feature_schema_id using urllib.parse.quote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ontology_endpoint,
headers=self.rest_endpoint_headers,
)
if response.status_code == 200:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if response.status_code == 200:
if response.status_code == requests.codes.ok:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return response.json()['unarchived']
else:
raise labelbox.exceptions.LabelboxError(
"Failed unarchive root feature schema node: ", response.text)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the convention other methods follow

Suggested change
"Failed unarchive root feature schema node: ", response.text)
"Failed unarchive root feature schema node, message: ", response.text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@kkim-labelbox kkim-labelbox left a comment

Choose a reason for hiding this comment

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

Could you add a test case for this function? Other than that, left a couple small comments!

def unarchive_feature_schema_node(self, ontology_id: str,
root_feature_schema_id: str) -> bool:
"""
Returns true if the root feature schema node was successfully unarchived, false otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we specify in the docstring here that only root level feature schemas are unarchivable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

headers=self.headers,
)
if response.status_code == requests.codes.ok:
return response.json()['unarchived']
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably fine to return None here, since it'll just return true when successfully unarchived anyway. Unless there's a case where unarchived = false

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'll return false if the user passes in a root feature schema id that does not belong to the ontology.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the response is false, maybe we can just raise an exception then with an error message indicating that the unarchive was unsuccessful? I know the docstring specifies the behavior, but customers may not necessarily check for the output of this function, so it may be safer to throw an explicit message indicating the behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

)
if response.status_code == requests.codes.ok:
unarchived = bool(response.json()['unarchived'])
if (unarchived == False):
Copy link
Contributor

Choose a reason for hiding this comment

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

You cna do:

if not unarchived

Copy link
Contributor

Choose a reason for hiding this comment

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

actually I would just do

  if not bool(response.json()['unarchived']):
    raise labelbox.exceptions.LabelboxError(
                    "Failed unarchive the feature schema.")
  return

and set the function return type to None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tim-Kerr Tim-Kerr merged commit 1f511f6 into develop Mar 10, 2023
@Tim-Kerr Tim-Kerr deleted the PTDT-1108 branch March 10, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants