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

add helpers and update docs #38

Merged
merged 13 commits into from
Aug 21, 2019
Merged

add helpers and update docs #38

merged 13 commits into from
Aug 21, 2019

Conversation

indirectlylit
Copy link
Contributor

@indirectlylit indirectlylit commented Jul 3, 2019

Adds some new convenience methods for working with embedded collection view blocks, and updates documentation.

See eff303d for an overview of changes.

Feedback on the API is welcome

@indirectlylit indirectlylit changed the title update links in readme to point to actual classes add helpers and update docs Jul 7, 2019
Copy link
Contributor Author

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

@jamalex left some notes inline

notion/block.py Outdated Show resolved Hide resolved
view.set("collection_id", collection.id)
cvb.set("collection_id", collection.id)
cvb.set("view_ids", [view.id])
collection = client.get_collection(client.create_record("collection", parent=page, schema=get_collection_schema()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

switching the parent from cvb to page did not seem to matter

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, maybe it's not used much internally. Still good to follow what Notion does internally, though, whenever we can -- in case there's some function that does end up using it. (parent is definitely used for collection records, in any case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a circular reference problem I'm not sure how to bootstrap:

collection = client.get_collection(client.create_record("collection", parent=cvb)
cvb = page.children.add_new(CollectionViewBlock, collection=collection)

Copy link
Owner

Choose a reason for hiding this comment

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

I'm guessing the reason a collection is attached to a CVB is so it knows where the "primary/home location" of the collection is, so it can show stuff like this:

image

The circularity issue does seem puzzling, so I looked into it a bit. Turns out Notion is tricksy when it comes to this. It avoids the issue by the combination of the following two facts:

  1. UUIDs in Notion are generated on the client side, and are random. You can choose whatever you want (as long as it's not taken on that table).
  2. It can make transaction requests, with multiple operations across numerous records taking place as part of a single atomic API request.

So what it does is create the database page (or inline block), the view, and the collection as a single transaction. So then it doesn't matter that the UUID references are circular, as it can already know what the Collection ID is when putting together the initial View data, and vice versa.

I'm wondering whether a decent idiom for this in notion-py might be to automatically create a Collection when you create a CollectionViewBlock/CollectionViewPageBlock or perhaps better a CollectionView, when you don't pass in an existing Collection. Then, we can internally handle the transaction/circular reference stuff.

What do you think?

Here's an example of the main pieces in the transaction, with the actual IDs replaced with placeholders for ease of reading:

Create the collection view page.

    {
      "id": "<COLLECTION_VIEW_PAGE_ID>",
      "table": "block",
      "path": [],
      "command": "update",
      "args": {
        "id": "<COLLECTION_VIEW_PAGE_ID>",
        "type": "collection_view_page",
        "collection_id": "<COLL_ID>",
        "view_ids": [
          "<VIEW_ID>"
        ],
        "properties": {},
        "created_time": 1566346433677,
        "last_edited_time": 1566346433677
      }
    },

Create the view.

    {
      "id": "<VIEW_ID>",
      "table": "collection_view",
      "path": [],
      "command": "update",
      "args": {
        "id": "<VIEW_ID>",
        "version": 0,
        "type": "table",
        "name": "Default View",
        "format": {
          "table_properties": [{"property": "title", "visible": true, "width": 276}, {"property": "4FoW", "visible": true}, {"property": "6xO9", "visible": true}],
          "table_wrap": true
        },
        "query": {
          "aggregate": [
            {
              "id": "count",
              "property": "title",
              "type": "title",
              "aggregation_type": "count",
              "view_type": "table"
            }
          ]
        },
        "page_sort": [],
        "parent_id": "<COLLECTION_VIEW_PAGE_ID>",
        "parent_table": "block",
        "alive": true
      }
    },

Create the collection.

    {
      "id": "<COLL_ID>",
      "table": "collection",
      "path": [],
      "command": "update",
      "args": {
        "id": "<COLL_ID>",
        "schema": {
          "title": {
            "name": "Name",
            "type": "title"
          },
          "4FoW": {
            "name": "Tags",
            "type": "multi_select"
          },
          "6xO9": {
            "name": "Files",
            "type": "file"
          }
        },
        "format": {
          "collection_page_properties": [
            {
              "property": "4FoW",
              "visible": true
            },
            {
              "property": "6xO9",
              "visible": true
            }
          ]
        },
        "parent_id": "<COLLECTION_VIEW_PAGE_ID>",
        "parent_table": "block",
        "alive": true
      }
    }```

Copy link
Owner

Choose a reason for hiding this comment

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

As discussed separately, I'll merge this and then we can keep thinking about nice idioms for this.

notion/smoke_test.py Show resolved Hide resolved
notion/block.py Outdated Show resolved Hide resolved
Copy link
Owner

@jamalex jamalex left a comment

Choose a reason for hiding this comment

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

Thanks -- approved! Left a couple small suggestions in case you want to do any of them, then we can merge.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
notion/block.py Show resolved Hide resolved
notion/block.py Show resolved Hide resolved
notion/block.py Outdated Show resolved Hide resolved
notion/block.py Outdated Show resolved Hide resolved
notion/block.py Show resolved Hide resolved
view.set("collection_id", collection.id)
cvb.set("collection_id", collection.id)
cvb.set("view_ids", [view.id])
collection = client.get_collection(client.create_record("collection", parent=page, schema=get_collection_schema()))
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, maybe it's not used much internally. Still good to follow what Notion does internally, though, whenever we can -- in case there's some function that does end up using it. (parent is definitely used for collection records, in any case)

run_smoke_test.py Show resolved Hide resolved
@indirectlylit
Copy link
Contributor Author

indirectlylit commented Aug 20, 2019

Copy link
Owner

@jamalex jamalex left a comment

Choose a reason for hiding this comment

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

Yay, thanks!

@jamalex jamalex merged commit 5d75238 into jamalex:master Aug 21, 2019
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.

None yet

2 participants