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

Adds 'todo list list' command. Closes #1609 #1616

Closed
wants to merge 1 commit into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented May 24, 2020

Closes #1609

@coveralls
Copy link

coveralls commented May 24, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 07045ad on ypcode:issue/1609-list-todo-task-list into 20117d6 on pnp:dev.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented May 24, 2020

Hero! 👏 🎉 We'll go through it shortly 👏

@garrytrinder garrytrinder changed the title todo list list command Adds 'todo list list' command. Closes #1609 May 24, 2020
@waldekmastykarz waldekmastykarz changed the base branch from dev to master May 31, 2020
@garrytrinder
Copy link
Member

garrytrinder commented Jun 22, 2020

This pull request has been converted to draft to signify that it is still a work in progress as we wait for the Microsoft Graph team to fully release the To Do APIs.

More Information

@garrytrinder garrytrinder marked this pull request as draft Jun 22, 2020
@garrytrinder
Copy link
Member

garrytrinder commented Aug 22, 2020

The To Do APIs have now been officially released to the Graph Beta endpoint with documentation, so this PR can now be progressed.

https://docs.microsoft.com/en-us/graph/api/resources/todo-overview?view=graph-rest-beta

@garrytrinder
Copy link
Member

garrytrinder commented Aug 22, 2020

The To Do APIs have now been officially released to the Graph Beta endpoint with documentation, so this PR can now be progressed.

https://docs.microsoft.com/en-us/graph/api/resources/todo-overview?view=graph-rest-beta

@ypcode
Copy link
Contributor Author

ypcode commented Aug 23, 2020

With all the recent changes in the CLI itself, I don't know what it implies yet, please let me know what I should change if any :)

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 23, 2020

@ypcode if you can verify that the command still works in your fork without updating the CLI (basically to check if the Graph API works the same as when you built it), then we can take care of the changes in the CLI. Deal? 🙂

@ypcode
Copy link
Contributor Author

ypcode commented Aug 23, 2020

Deal! Let me have a quick look later today :)

@ypcode
Copy link
Contributor Author

ypcode commented Aug 23, 2020

@waldekmastykarz I am checking it, so far so good, but some new options have been added
(e.g. initially only lists of current user were supported, now lists of a specific user seem to be supported as well).

I will verify everything as-is is still working in the 4 CRUD commands, after you kindly do the CLI v3 related changes, I will refork fresh and clean and bring the changes for the latest capabilities, what do you think ?

There are now also the CRUD API for tasks, (https://docs.microsoft.com/en-us/graph/api/todotasklist-post-tasks?view=graph-rest-beta)
can you please assign those commands to me ?

@ypcode
Copy link
Contributor Author

ypcode commented Aug 23, 2020

There is however a new Permission to add to the CLI AAD app registration
(e.g. Tasks.ReadWrite https://docs.microsoft.com/en-us/graph/api/todo-post-lists?view=graph-rest-beta#permissions )
But I am actually not sure where that must be changed in the source code ?

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 23, 2020

I will verify everything as-is is still working in the 4 CRUD commands, after you kindly do the CLI v3 related changes, I will refork fresh and clean and bring the changes for the latest capabilities, what do you think ?

Excellent! Let's get these PRs in and we can always extend them with additional options.

There are now also the CRUD API for tasks, (https://docs.microsoft.com/en-us/graph/api/todotasklist-post-tasks?view=graph-rest-beta) can you please assign those commands to me ?

Do we have a spec for them already? If not, let's start with the spec

There is however a new Permission to add to the CLI AAD app registration

We'll add them to the AAD app registration. We don't manage permissions in code

@waldekmastykarz waldekmastykarz marked this pull request as ready for review Aug 23, 2020
@waldekmastykarz waldekmastykarz self-assigned this Aug 26, 2020
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Nicely done, with a couple of adjustments I've done when merging the PR 👍

Examples:

Get the list of Microsoft To Do task lists
`);
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 27, 2020

Choose a reason for hiding this comment

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

We're missing the code example here

`
Examples:

Get the list of Microsoft To Do task lists
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 27, 2020

Choose a reason for hiding this comment

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

This should be indented


interface Options extends GlobalOptions { }

class TodoListListCommand extends GraphCommand {
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 27, 2020

Choose a reason for hiding this comment

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

Since this command is meant to retrieve all items, it would be better to base it on the GraphItemsListCommand class which implements retrieving all items, including paging in case there are more results than fit in a single response.

url: `${this.resource}/beta/me/todo/lists`,
headers: {
accept: 'application/json;odata.metadata=none',
'content-type': 'application/json'
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 27, 2020

Choose a reason for hiding this comment

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

No need to specify the content type as it's a GET request

request
.get(requestOptions)
.then((res: any): void => {
cmd.log(res.value);
Copy link
Member

@waldekmastykarz waldekmastykarz Aug 27, 2020

Choose a reason for hiding this comment

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

Let's not output all properties by default, because they won't fit the terminal window. Instead, for the default text output let's show just the displayName and id and then in output JSON we can show everything we got from the API.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 27, 2020

Merged manually. Thank you! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: List Microsoft To Do task lists
4 participants