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 set' command. Closes #1612 #1615

Closed
wants to merge 1 commit into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented May 24, 2020

Closes #1612

@coveralls
Copy link

coveralls commented May 24, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 2137f25 on ypcode:issue/1612-set-todo-task-list into 20117d6 on pnp:dev.

@ypcode ypcode force-pushed the issue/1612-set-todo-task-list branch from 091a042 to 2137f25 Compare May 24, 2020 12:43
@waldekmastykarz
Copy link
Member

👏 Cool! We'll review it shortly

@garrytrinder garrytrinder changed the title todo list set command Adds 'todo list set' command. Closes #1612 May 24, 2020
@waldekmastykarz waldekmastykarz changed the base branch from dev to master May 31, 2020 09:39
@garrytrinder garrytrinder marked this pull request as draft June 22, 2020 21:27
@garrytrinder
Copy link
Member

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
Copy link
Member

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

@waldekmastykarz For this, the API endpoint and payload are still identical, this can be refactored to align the CLI v3 changes
(Please mind the new permission needed Tasks.ReadWrite)

@waldekmastykarz waldekmastykarz marked this pull request as ready for review August 23, 2020 13:34
@waldekmastykarz waldekmastykarz self-assigned this Aug 27, 2020
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Nicely done with a few adjustments I did when merging the PR 👍


request
.patch(requestOptions)
.then((res: any): void => {
Copy link
Member

Choose a reason for hiding this comment

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

Since we're not using the response for anything, we don't need to specify it here


public getTelemetryProperties(args: CommandArgs): any {
const telemetryProps: any = super.getTelemetryProperties(args);
telemetryProps.name = typeof args.options.name !== 'undefined';
Copy link
Member

Choose a reason for hiding this comment

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

Both properties are required, so there is no point in including them in the telemetry because they would always have value of true

description: `The ID of the list to update`
},
{
option: '-n, --name <name>',
Copy link
Member

Choose a reason for hiding this comment

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

Let's update this property to newName. In the future we want to allow users to retrieve resources by their name, so to avoid breaking changes in the future, let's pick a neutral name for this option.

`
Examples:

Rename the list with Id "AAMkAGI3NDhlZmQzLWQxYjAtNGJjNy04NmYwLWQ0M2IzZTNlMDUwNAAuAAAAAACQ1l2jfH6VSZraktP8Z7auAQCbV93BagWITZhL3J6BMqhjAAD9pHIhAAA=" to "My updated task list"
Copy link
Member

Choose a reason for hiding this comment

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

This should be indented further

assert.notEqual(command.description, null);
});

it('updates a To Do task', (done) => {
Copy link
Member

Choose a reason for hiding this comment

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

Updates a list, not a task

});
});

it('updates a To Do task (verbose)', (done) => {
Copy link
Member

Choose a reason for hiding this comment

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

Updates To Do list, not a task

options: {
debug: false,
id: "AAMkAGI3NDhlZmQzLWQxYjAtNGJjNy04NmYwLWQ0M2IzZTNlMDUwNAAuAAAAAACQ1l2jfH6VSZraktP8Z7auAQCbV93BagWITZhL3J6BMqhjAAD9pHIjAAA=",
name: null
Copy link
Member

Choose a reason for hiding this comment

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

In JS not set means undefined, not null

const actual = (command.validate() as CommandValidate)({
options: {
debug: false,
id: null,
Copy link
Member

Choose a reason for hiding this comment

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

In JS not set means undefined, not null

@waldekmastykarz
Copy link
Member

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: Update a Microsoft To Do task list
4 participants