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

[IO-1278][IO-1277] Core Items/Folders apis #691

Merged
merged 16 commits into from
Oct 20, 2023
Merged

[IO-1278][IO-1277] Core Items/Folders apis #691

merged 16 commits into from
Oct 20, 2023

Conversation

Nathanjp91
Copy link
Contributor

Problem

No ability to retrieve or manage items and folders

Solution

  • Introduce basic item object
  • backend calls for item and folders endpoints

Changelog

Beggining of support for items object, including apis

@linear
Copy link

linear bot commented Oct 19, 2023

IO-1278 Core: Retrieve Items

https://www.notion.so/v7labs/Item-Management-Functionality-Scoping-df5f846a135e46e1bf31c473afda32ee?pvs=4#ec8828e354a74e64a668a83b23a4a97a

get_item(client, id)
get_items(client, params)

Create methods for:

  • Get all items
  • Get item by ID
  • Get item ids
  • Get item folder

IO-1277 Core: Item Object

Class Item(BaseModel):
  ...

return [
[edge.id, edge.source_stage_id, edge.target_stage_id]
for edge in self._element.edges
if edge.source_stage_id and edge.target_stage_id
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JBWilkie source_stage_id and target_stage_id are optional so they can be None. Which means this comprehension without the if check actually returns a List[List[UUID | None]]. Let me know if this change doesn't make sense though and you want the None's

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't see a use case for why the Nones would be needed but you never know

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually changed this so that it returns the WFEdgeCore items instead of the IDs within them, not sure why I didn't do this originally

This means we don't need to worry about this if check

Copy link
Contributor

@owencjones owencjones left a comment

Choose a reason for hiding this comment

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

Looking good, hold off merge until we've worked out data structs in call later, and potentially until @JBWilkie responds on the points about none filtering, although I don't think that's as blocking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this stuff especially 😉

return v


class ItemSlot(DefaultDarwin):
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 main one I've changed in my branch



class Item(DefaultDarwin):
name: str
Copy link
Contributor

Choose a reason for hiding this comment

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

And this is where we'll have to figure it out between us.

@pytest.fixture
def base_items_json(base_items: List[Item]) -> List[dict]:
items = [item.dict() for item in base_items]
# json library doesn't support UUIDs so need to be str'd
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, what's with this? Every time I hit this, I'm tempted to write:

class JsonableUUID(UUID):
    ...fix for this...

return [
[edge.id, edge.source_stage_id, edge.target_stage_id]
for edge in self._element.edges
if edge.source_stage_id and edge.target_stage_id
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't see a use case for why the Nones would be needed but you never know

@Nathanjp91 Nathanjp91 merged commit f5992b4 into master Oct 20, 2023
13 checks passed
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.

3 participants