Made it work with the `2023-10` version of the API #116

willnaoosmith · 2023-10-05T20:20:31Z

Hi there!

I made this wrapper work the new API, which will be live on 17 of January 2024.
There are some other changes that are from the repository that I forked, since it was more recent than the original repository.

This should fix (or at least help with) the #115 issue.
Tested on a test board and it works fine!

Changes I made:

The headers option, that I mentioned before (Will not be required to use the new API version after 17 of January).

monday = MondayClient(
    token = "YourTokenHere",
    headers = {'API-Version' : '2023-10'}
)

The fetch_updates_for_item query doesn't use the board ID parameter anymore, so it should be like this:

monday.updates.fetch_updates_for_item(item_id="ItemIDHere", limit="LimitHere")

the fetch_items_by_column_value uses items_page_by_column_values now instead of the items_by_column_values, but this does not change anything about how it should be used.

monday.items.fetch_items_by_column_value(board_id='BoardIDHere',  column_id="ColumnIDHere", value="ColumnValueHere")

To test it, install it by running pip install git+https://github.com/willnaoosmith/monday.git and change the headers to use the 2023-10 version.

…ce_id Feature/create board by workspace

Feature/duplicate board query

Added: Create and Update columns

Added: Exception for catching Monday API query errors

Added: Custom queries resource

Added: Me queries resource

Added: Query for deleting updates

chdastolfo · 2023-11-09T19:35:58Z

@willnaoosmith Thanks so much for doing this!! We'll take a look ASAP!

pevner-p2

@chdastolfo, I'm generally 👍 with these changes, though I'm mostly praying that the tests are reliable and trusting that the author has tested sufficiently on the new API version.

However, there's some formatting stuff that's upsetting the ProdPerfect linter. This is coming from the author's private fork, so I can't just mess fix it up on that branch, and I'm not going to ask them to fix up stuff specific to our linting rules. Instead, I assume we'll accept this PR, and then I'll throw up another PR just for the formatting stuff. Maybe that can include the deployment related changes (e.g. updated contributor list, incremented version).

Hopefully we can coordinate a deployment soon, maybe we bring in rhymiz if you want?

Anyway, I've left a few comments, nothing blocking, most of them are just me ranting. Glad to get this cooking.

pevner-p2 · 2023-11-09T23:22:34Z

monday/graphqlclient/client.py

            return response.json()
        except requests.HTTPError as e:
            print(e)
            raise e
+        except MondayQueryError as ex:


[non-blocking] This could be collapsed into the previous except block using a tuple, i.e. except (requests.HTTPError, MondayQueryError) as e.

pevner-p2 · 2023-11-09T23:24:56Z

monday/client.py



 class MondayClient:
-    def __init__(self, token):
+    def __init__(self, token, headers={}):


[non-blocking] Given that this header is there primarily to handle the API change, are we planning on getting rid of this header parameter after Monday removes the old API?

They probably will still update the API, and still let us set the API version on the headers, so at least I think it would be helpful to have the option to set the headers to every request nonetheless.

We depend on them to make the changes, so we'll never know.

Also, looks like this is the only commented code that I actually made, so this is the only one I can say about.

Defaulting a parameter to a mutable value is very dangerous:

client = MondayClient(token = "abc") client2 = MondayClient(token = "MMM") client.boards.fetch_columns_by_board_id("dsadsaa") client2.boards.client.headers # {'Authorization': 'abc', 'Content-Type': 'application/json'} wrong API key

An alternative would be to pass the desired version api_version="2023-10" to the constructor

Happy to hear your thoughts but here's why I think it'd be better:

It makes it more obvious what we're trying to achieve with this param, since that's the only (current) logical use for the headers, outside of internal stuff like Content-Type etc.

By keeping the version on e.g. self.api_version, it would allow to keep the old stuff working if someone wants to use the old API for the time being. For example the added items_page() in queries could be added only if self.api_version == '2023-10' etc

See my comment above about mutating a default class param. A string doesn't have that issue

Users of the lib don't need to know what the headers look like (yes, it's very simple to document, but still)

My $0.02: I'm still of the opinion that we should consider burning this parameter entirely come 16 Jan 2024, and have library version labels indicating what's happening.

Given that Monday is dropping support for the old version on 15 Jan, this would be a legacy parameter 3 months after release. There's certainly a chance that they'll do more versioning in the future, but I'd rather be handling it implicitly, through our client library versioning, than try to support multiple API versions in the same client library. I appreciate that their future versioning might be more incremental, but given that they're using date versioning, rather than semantic versioning, they're permitted hard breaks with each iteration.

Also, this is how I wish we were doing it now, but I'm not one to look a gift PR in the mouth, given that we've got a relatively short timeline for cutting this library over.

My $0.02: I'm still of the opinion that we should consider burning this parameter entirely come 16 Jan 2024, and have library version labels indicating what's happening.

Given that Monday is dropping support for the old version on 15 Jan, this would be a legacy parameter 3 months after release. There's certainly a chance that they'll do more versioning in the future, but I'd rather be handling it implicitly, through our client library versioning, than try to support multiple API versions in the same client library. I appreciate that their future versioning might be more incremental, but given that they're using date versioning, rather than semantic versioning, they're permitted hard breaks with each iteration.

Also, this is how I wish we were doing it now, but I'm not one to look a gift PR in the mouth, given that we've got a relatively short timeline for cutting this library over.

I see your point, because they are literally deprecating the old API. Maybe there is simply no need for an extra parameter. In any case, it should definitely not be declared with a mutable dict as default params value :)

pevner-p2 · 2023-11-09T23:29:43Z

monday/exceptions.py

@@ -1,2 +1,6 @@
 class MondayError(RuntimeError):
    pass
+
+
+class MondayQueryError(Exception):


[very non-blocking] I don't know why we inherit from RuntimeError for MondayError, but maybe we should follow that pattern? I think it's off, personally, but I don't have the context. Alternately, this could inherit from MondayError, but that also seems wrong, as it doesn't seem to be used anywhere. Maybe it can go away? I don't know.

I genuinely do not know or remember. That can almost certainly go away.

Technically, MondayError (base exception class for this library) should inherit from Exception and MondayQueryError should inherit from MondayError. This makes it very easy to handle "all" potential exceptions that may be thrown inside the monday client by calling code:

try: pass except MondayError: pass

pevner-p2 · 2023-11-09T23:31:41Z

monday/query_joins.py

+    folder_id = folder_id if folder_id else None
+    keep_subscribers = keep_subscribers if keep_subscribers else False
+
+    params = """board_id: %s, duplicate_type: %s, board_name: \"%s\"""" % (


[very non-blocking] Why does no one in this repo use f-strings?!

[edit] And then, randomly, later on, they do!!

The non f-strings were almost certainly my fault when I first wrote this, what feels like eons ago, and then f-strings were added later by either myself or contributors. I've been meaning to go through and clean this up.

pevner-p2 · 2023-11-09T23:33:02Z

monday/resources/boards.py

+    ):
+        query = duplicate_board_query(board_id, duplicate_type, board_name, workspace_id, folder_id, keep_subscribers)
+        return self.client.execute(query)
+        return self.client.execute(query)


[non-blocking, but c'mon] Dupe.

Yeah, can we delete this?

willnaoosmith · 2023-11-10T10:05:16Z

@pevner-p2 I wasn't that much experienced with the project, and I also forked a fork of the original project (As of right now, that fork is 21 commits ahead of the original).
So it might not have the best formatting and/or the most reliable way that might serve everybody.

Worked for me at least, so I hope it helps more people haha

JUSTFUN0368 · 2023-11-10T18:08:09Z

Since you guys are working on migrating to the new API version, you should adjust the get_board_items_query() and get_items_by_group_query() to nest items within the items_page object.

chdastolfo

👍 @pevner-p2 I agree. Would like to get these in to solve the immediate issue and then go back and do some cleanup and make things agree with the linter. I have to do some work to re-acquaint myself to this and don't want to be a blocker.

chdastolfo · 2023-11-11T17:47:29Z

Since you guys are working on migrating to the new API version, you should adjust the get_board_items_query() and get_items_by_group_query() to nest items within the items_page object.

I haven't used Monday in a little bit. Don't want to block this PR, but agree this should be done. Would you mind filing an issue for this?

chdastolfo · 2023-11-11T18:27:55Z

monday/query_joins.py

+
+    return query
+
+def duplicate_board_query(board_id: int, duplicate_type: DuplicateTypes):


Why does this have the same name as the method above? It's going to override the above function, as it's defined later in the file.

chdastolfo · 2023-11-11T19:04:50Z

monday/resources/columns.py

+        super().__init__(token, headers)
+
+    def create_column(self, board_id):
+        query = get_columns_by_board_query(board_id)


This seems like the wrong query. Shouldn't this be calling create_column from inside the query file?

Found some blocking issues.

matiboy · 2023-11-13T06:52:14Z

monday/graphqlclient/client.py

        if self.token is not None:
            headers[self.headername] = '{}'.format(self.token)

        if variables is None:
-            headers['Content-Type'] = 'application/json'
+            if 'Content-Type' not in headers:
+                headers['Content-Type'] = 'application/json'


Thanks for the great work @willnaoosmith and very happy to see it was picked up by the authors so quickly. Just a comment:

Mutating the headers seems like a bad idea; headers are not copied at any point in time in this PR, so they are shared across all Resources.

client = MondayClient(token = "abc",headers = {'API-Version' : '2023-10'}) client.items.client.headers # {'API-Version': '2023-10'} client.boards.fetch_columns_by_board_id("lala") # Note this is on client.boards not client.items client.items.client.headers """ Output {'API-Version': '2023-10', 'Authorization': 'abc', 'Content-Type': 'application/json'} """

Good catch. This was caught by our internal linter, so I'd glossed over it as a syntactic issue, but this is definitely a genuine bug.

matiboy

Thanks for the great work @willnaoosmith and very happy to see it was picked up by the authors so quickly.

There are however some issues that would need to be addressed. I hope you consider the option of setting api_version in constructor rather than passing headers, any maybe @pevner-p2 and @chdastolfo can weigh in on that option.

Thanks and sorry for the "long" review

matiboy · 2023-11-13T07:25:20Z

monday/client.py



 class MondayClient:
-    def __init__(self, token):
+    def __init__(self, token, headers={}):


An alternative would be to pass the desired version api_version="2023-10" to the constructor

Happy to hear your thoughts but here's why I think it'd be better:

It makes it more obvious what we're trying to achieve with this param, since that's the only (current) logical use for the headers, outside of internal stuff like Content-Type etc.

By keeping the version on e.g. self.api_version, it would allow to keep the old stuff working if someone wants to use the old API for the time being. For example the added items_page() in queries could be added only if self.api_version == '2023-10' etc

See my comment above about mutating a default class param. A string doesn't have that issue

Users of the lib don't need to know what the headers look like (yes, it's very simple to document, but still)

matiboy · 2023-11-13T07:41:20Z

monday/query_joins.py

+    board_name = board_name if board_name else ""
+    workspace_id = workspace_id if workspace_id else None
+    folder_id = folder_id if folder_id else None
+    keep_subscribers = keep_subscribers if keep_subscribers else False


For the 4 lines above,

workspace_id and folder_id are already defaulting to None

why not set the defaults in the function definition board_name: str = "", keep_subscribers: bool = False ?

where possible, I'd avoid None since the typing is then incorrect (int = None will not be well liked); as we know that real workspace_id and folder_id will never be "0" why not set that as "falsy" value? Guess that's more of a preference

agree

agree

Type annotations should be int | None or Union[int, None]

Then bellow:

if workspace_id is not None: # do something if folder_id is not None: # do something

matiboy · 2023-11-13T07:41:38Z

monday/query_joins.py

+    )
+
+    if workspace_id:
+        params += """,  workspace_id: %s"""


The values are already merged in the lines above, so wouldn't this extra params remain unmerged (workspace_id is never used)

matiboy · 2023-11-13T07:43:11Z

monday/resources/columns.py

+
+class ColumnsResource(BaseResource):
+    def __init__(self, token, headers):
+        super().__init__(token, headers)


Unnecessary since no change from super

Though I notice this is likely because other Resource sub classes (from original code) have an unnecessary super().__init__ too; I think would be nice to remove but of course not so important

matiboy · 2023-11-13T07:45:51Z

monday/query_joins.py

+    board_id: int, column_title: str, column_type: None, defaults: dict[str, any] = None, description: str = None
+):
+    """C```reate a new column by board ID
+


Extra "```"

matiboy · 2023-11-13T07:46:46Z

monday/query_joins.py

+    Return:
+        [Dict[str,Any]]: Column's Monday details after creation
+    """
+    defaults = defaults if defaults else ""


defaults is typed as a dict? Should it be an empty string if not provided?

@matiboy you're calling out the correct thing -- I think the reason we're able to get away with dict or str is because monday_json_string converts the dictionary into a string -- I do agree that for correctness, this should be:

defaults = defaults or {}

matiboy · 2023-11-13T07:47:07Z

monday/query_joins.py

+        [Dict[str,Any]]: Column's Monday details after creation
+    """
+    defaults = defaults if defaults else ""
+    description = description if description else ""


Might as well declare "" as default value in function definition

matiboy · 2023-11-13T07:49:12Z

monday/query_joins.py

@@ -167,6 +166,75 @@ def delete_item_query(item_id):
    return query


+# COLUMNS RESOURCE QUERIES
+def create_column(
+    board_id: int, column_title: str, column_type: None, defaults: dict[str, any] = None, description: str = None


column_type typed as None is likely erroneous. ColumnTypes seems to be the correct type (from doc string below)

matiboy · 2023-11-13T07:55:46Z

monday/query_joins.py

+    else:
+
+        query = """mutation{
+            create_column(board_id: %s, title: "%s", description: "%s", column_type: %s, defaults: %s) {


Just a warning as the column_type seems to have been meant to be typed to ColumnTypes, string interpolation of enums has changed somewhere in 3.10 I believe:

class A(Enum): ABC = "123" """enum value: %s""" % (A.ABC,) # wrong: 'From enum value: A.ABC' """enum value: %s""" % (A.ABC.value,) # correct: enum value: 123

rhymiz · 2023-11-15T01:31:49Z

monday/graphqlclient/client.py

            files = [
                ('variables[file]', (variables['file'], open(variables['file'], 'rb')))
            ]

        try:
            response = requests.request("POST", self.endpoint, headers=headers, data=payload, files=files)
            response.raise_for_status()
+            self._catch_error(response)


You're not technically "catching" any errors here, you're checking for the present of any errors, so we should probably rename the method to _raise_if_error()

albcl and others added 24 commits June 18, 2022 19:12

Added "create board by workspace id"

e9c2d80

Update readme

748f418

Added "duplicate board" query and type hint

5480228

Readme updated

5dc386d

Merge pull request albcl#1 from albcl/feature/create_board_by_workspa…

ed8e29d

…ce_id Feature/create board by workspace

Added extra parameters for the query

873dda3

Merge branch 'master' into feature/duplicate_board_query

f9e3a6e

Merge pull request albcl#2 from albcl/feature/duplicate_board_query

feca139

Feature/duplicate board query

Added: Create and Update columns

4c191ff

Added: Custom queries resource

b5ec23c

Added: Me queries resource

60558fa

Added: Exception for catching Monday API query errors

b510b8a

Added: Query for deleting updates

90a51e6

Merge pull request albcl#3 from albcl/feature/columns_resource

f69e88a

Added: Create and Update columns

Merge pull request albcl#4 from albcl/feature/catch-monday-api-errors

972871c

Added: Exception for catching Monday API query errors

Merge branch 'master' into feature/custom_queries

7c75864

Merge pull request albcl#5 from albcl/feature/custom_queries

c17f5d8

Added: Custom queries resource

Merge branch 'master' into feature/me_queries

8b1d39b

Merge pull request albcl#6 from albcl/feature/me_queries

de0d28a

Added: Me queries resource

Merge pull request albcl#7 from albcl/feature/delete_update_query

5852191

Added: Query for deleting updates

Merge remote-tracking branch 'upstream/master'

4e6b327

Added headers functionality

6ae9403

done!

62167c1

Update README.md

b39e316

pevner-p2 requested a review from chdastolfo November 9, 2023 17:30

pevner-p2 requested a review from rhymiz November 9, 2023 23:13

pevner-p2 approved these changes Nov 9, 2023

View reviewed changes

chdastolfo previously approved these changes Nov 11, 2023

View reviewed changes

chdastolfo reviewed Nov 11, 2023

View reviewed changes

matiboy reviewed Nov 13, 2023

View reviewed changes

rhymiz reviewed Nov 15, 2023

View reviewed changes

pevner-p2 mentioned this pull request Nov 15, 2023

Updated "Made it work with the 2023-10 version of the API" #117

Merged

pevner-p2 merged commit b39e316 into ProdPerfect:master Nov 22, 2023


		return query

		def duplicate_board_query(board_id: int, duplicate_type: DuplicateTypes):

Made it work with the 2023-10 version of the API #116

Made it work with the 2023-10 version of the API #116

Conversation

willnaoosmith commented Oct 5, 2023

chdastolfo commented Nov 9, 2023

pevner-p2 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

pevner-p2 Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pevner-p2 Nov 9, 2023 • edited Loading

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

willnaoosmith commented Nov 10, 2023

JUSTFUN0368 commented Nov 10, 2023

chdastolfo left a comment

Choose a reason for hiding this comment

chdastolfo commented Nov 11, 2023

chdastolfo Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

chdastolfo Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiboy 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

matiboy Nov 13, 2023 • edited Loading

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

matiboy Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

matiboy Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

rhymiz Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Made it work with the `2023-10` version of the API #116

Made it work with the `2023-10` version of the API #116

pevner-p2 Nov 13, 2023 •

edited

Loading

pevner-p2 Nov 9, 2023 •

edited

Loading

chdastolfo Nov 11, 2023 •

edited

Loading

chdastolfo Nov 11, 2023 •

edited

Loading

matiboy Nov 13, 2023 •

edited

Loading

matiboy Nov 13, 2023 •

edited

Loading

matiboy Nov 13, 2023 •

edited

Loading

rhymiz Nov 15, 2023 •

edited

Loading