-
Notifications
You must be signed in to change notification settings - Fork 53
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
Feature/duplicate board query #61
base: master
Are you sure you want to change the base?
Conversation
Hi @albcl, thanks for your contribution! We'll have this reviewed ASAP! cc: @chdastolfo |
Feature/create board by workspace
I have also added some extra optional parameters for the query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@albcl Could you please update the README with details about each of these optional parameters? To be honest, I haven't used monday.com in a while 🤣 - I'm not clear on when you would use something like folder_id
or keep_subscribers
so some extra context there would be helpful.
Hey @albcl, following up here. I'd love to get this merged for you once you've addressed the comments. |
Hi everyone. Sorry about the delay but it has been a rather busy and hectic last few months. |
Hey @albcl, looks like the build is failing due to failing tests. |
* Added move item to group and archive item * Added tests for move item to group and archive item * Updated README.md * Update package version
* Added "create board by workspace id" * Update readme * Added missing type hints * Updated test * More custom type hints * Type hints for 'get_boards_query' * Fixed tests * Updated Readme and 'fetch_boards' method
* docs: update README.md [skip ci] * docs: create .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
* docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
* docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
* Add in limits and pages to items query * Add in unit test * Add in no params test * Update surface level api * Split up test * Split out gather func * Update README * CORRECTLY update README * bump version number Co-authored-by: Taylor Cochran <[email protected]>
* docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
With a little of a delay... but fixed! It was a matter of merging and updating some changes that were committed by #60 but missing here. |
if workspace_id: | ||
params += """, workspace_id: %s""" | ||
|
||
query = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it should be indented once more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to move it under the workspace_id
condition? That condition is meant to only add to the params variable the workspace_id
parameter for the query but it's not a mandatory condition for the query.
However, there is a clear issue there because there isn't a value passed for the %s
token, and the test didn't catch it because it was expecting to find a 1 and a 2 within the query, but both are included in the board_id
value (Its value is 12) That was a really bad test.
I have updated both the query and its test to fix these issues. I wonder if it had something to do with the merge when the branch was like +20 commits behind...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, now that I look at it again I'm not sure what I was talking about, it looks fine... disregard.
Good catch on test, and ty for fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved pending the resolution of the merge conflict.
@albcl We've migrated our docs over to Read the Docs - you can find README.rst
in the docs/
directory if you pull down the latest - could you move the README additions to that file? This should be good to go after that.
Friendly ping @albcl |
Bumps [certifi](https://github.com/certifi/python-certifi) from 2021.5.30 to 2022.12.7. - [Release notes](https://github.com/certifi/python-certifi/releases) - [Commits](certifi/python-certifi@2021.05.30...2022.12.07) --- updated-dependencies: - dependency-name: certifi dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
@rhymiz @chdastolfo with a massive delay, here is the updated PR once again. Apologies for leaving the PR hanging for a year 🤦 Many things have happened since then! |
What changed:
Added
duplicate_board()
function and a test for it.A types.py file has been included where type hints can be set up. In this case it stores the required duplication types (duplicate_board_with_structure / duplicate_board_with_pulses / duplicate_board_with_pulses_and_updates)
Also, Readme has been updated.
What resolves
Issue #46 - Will allow users to duplicate boards easily.