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 remove' command. Closes #1611 #1614

Closed
wants to merge 4 commits into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented May 24, 2020

Closes #1611

@coveralls
Copy link

coveralls commented May 24, 2020

Coverage Status

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

@ypcode
Copy link
Contributor Author

ypcode commented May 24, 2020

@garrytrinder oops... the previous commit was included in this PR... can you live with it ? Or do you really prefer me to get rid of it, if both PRs are merged, I guess it’s not worthy rework it but up to you :) I’ll wait for those to be merged to dev to continue the other ones to avoid this...

Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Seems like this PR sucked in todo list add. Would you mind removing them before we continue with the review to keep things clear?

@ypcode
Copy link
Contributor Author

ypcode commented May 24, 2020

Sure, let me rebase and get a clean PR :)

@ypcode ypcode force-pushed the issue/1610-remove-todo-task-list branch from ad31392 to 6f22396 Compare May 24, 2020
@ypcode
Copy link
Contributor Author

ypcode commented May 24, 2020

Here it go with a rewrite of the history @waldekmastykarz :D

@garrytrinder garrytrinder changed the title Issue #1611 remove todo task list Adds 'todo list remove' command. Closes #1611 May 24, 2020
@garrytrinder garrytrinder self-assigned this May 24, 2020
Copy link
Member

@garrytrinder garrytrinder left a comment

Thank you for the PR @ypcode

The command is shaping up nicely however I've added some comments, would you mind taking a look at them, thank you.

@ypcode
Copy link
Contributor Author

ypcode commented May 24, 2020

Hi @garrytrinder , I don't know if I am missing something... I don't see your review comments anywhere ?

@garrytrinder
Copy link
Member

garrytrinder commented May 24, 2020

Apologies @ypcode looks like my comments didn't get committed 🤦🏻‍♂️ Sorry for the trouble, I'll get them added ASAP.

@ypcode
Copy link
Contributor Author

ypcode commented May 24, 2020

no worries, I was just concerned I was not doing it right :) Take your time !

Copy link
Member

@garrytrinder garrytrinder left a comment

Sorry for the trouble @ypcode could you take a look at these comments, thanks 👍🏻

docs/manual/docs/cmd/todo/list/list-remove.md Outdated Show resolved Hide resolved
docs/manual/docs/cmd/todo/list/list-remove.md Outdated Show resolved Hide resolved
docs/manual/docs/cmd/todo/list/list-remove.md Outdated Show resolved Hide resolved
docs/manual/docs/cmd/todo/list/list-remove.md Outdated Show resolved Hide resolved
docs/manual/mkdocs.yml Outdated Show resolved Hide resolved
src/o365/todo/commands/list/list-remove.ts Show resolved Hide resolved
src/o365/todo/commands/list/list-remove.ts Outdated Show resolved Hide resolved
src/o365/todo/commands/list/list-remove.ts Outdated Show resolved Hide resolved
src/o365/todo/commands/list/list-remove.ts Outdated Show resolved Hide resolved
src/o365/todo/commands/list/list-remove.ts Outdated Show resolved Hide resolved
@ypcode
Copy link
Contributor Author

ypcode commented May 24, 2020

So ... :D Productive Sunday, I'll go to sleep a bit before starting a new week :D

getListId().then(listId => {
if (!listId) {
cb(new CommandError(`The list ${args.options.name} cannot be found`));
return;
}
Copy link
Member

@garrytrinder garrytrinder May 24, 2020

Choose a reason for hiding this comment

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

This was a comment that was missed 😬

If we supply the id of a task list that doesn't exist, the Graph API doesn't throw an error, instead it returns a 204 success response, thus resulting in a false positive.

We should check the validity of the id that is passed and throw an error in the same way we do it for when you pass the name of a task list that doesn't exist.

Copy link
Contributor Author

@ypcode ypcode May 24, 2020

Choose a reason for hiding this comment

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

Okay, something that we will have to take care of...
However some observations with the beta API:

  • with any Id to https://graph.microsoft.com/beta/me/todo/lists/?$filter=id eq 'AAMkAGI3NDhlZmQzLWQxYjAtNGJjNy04NmYwLWQ0M2IzZTNlMDUwNAAuAAAAAACQ1l2jfH6VSZraktP8Z7auAQCbV93BagWITZhL3J6BMqhjAAD9pHIjAAA='
    image
  • with a quite different Id https://graph.microsoft.com/beta/me/todo/lists/AADDDGI3NDhlZmQzLWQxYjAtNGJjNy04NmYwLWQ0M2IzZTNlMDUwNAAuAADDDDCQ1l2jfH6VSZraktP8Z7auAQCbV93BagWITZhL3J6BMqhjAAD9pHIjAAA=
    image
  • But worst ! with a slightly mispelled Id https://graph.microsoft.com/beta/me/todo/lists/AAMkAGI3NDhlZmQzLWQxYjAtNGJjNy04NmYwLWQ0M2IzZTNlMDUwNAAuAAAAAACQ1l2jfH6VSZraktP8Z7auAQCbV93BagWITZhL3J6BMqhjAAD9pHIjDDD= (Only the 3 last characters mispelled)
    image

Bottom line is, the APIs are clearly not yet reliable for IDs....

Copy link
Contributor Author

@ypcode ypcode May 24, 2020

Choose a reason for hiding this comment

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

Actually... Identifying by the display name is more reliable... Maybe we should get rid of the Id option altogether for now ?

Copy link
Member

@garrytrinder garrytrinder May 24, 2020

Choose a reason for hiding this comment

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

Interesting observations, thank you for the research and context on this.

The third example is worrying if you get a different task list returned that doesn't match the Id supplied in the request, we should definitely relay this back to the Graph team.

My test just passed the same Id of the list I had just deleted, essentially running the command twice and expecting an error on the second try.

Based on your observations, we could call GET /beta/me/todo/lists/<listId> and then confirm that the success response does not match with the Id supplied and throw an error, however this is not ideal. Thoughts?

Copy link
Contributor Author

@ypcode ypcode May 25, 2020

Choose a reason for hiding this comment

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

It looks pretty straightforward! That should work ! Let’s give it a try!

Copy link
Member

@garrytrinder garrytrinder May 25, 2020

Choose a reason for hiding this comment

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

@pnp/office-365-cli-maintainers are you happy with this approach?

Copy link
Contributor Author

@ypcode ypcode May 25, 2020

Choose a reason for hiding this comment

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

I still don't get where the Id come from when you mispell it and it returns something that exists with neither the original Id nor the Id you specified XD ... But anyway, with checking if the argument Id and the return Id are the same, we should be okay, I tested it and it works... I am adapting the .spec file

Copy link
Contributor Author

@ypcode ypcode May 25, 2020

Choose a reason for hiding this comment

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

@garrytrinder as you mentioned, with the Id of the list you've just deleted, you get some expected 404 which is handled by the command error handler and prints out Item not found, if you misspell the Id, you get this strange behavior. but then, it goes through this code I just changed

    if (!listId) {
          if (args.options.name) {
            cb(new CommandError(`A task list with name ${args.options.name} cannot be found`));
          } else if (args.options.id) {
            cb(new CommandError(`A task list with Id ${args.options.id} cannot be found`));
          }
          return;
        }

To be consistent to the user, should we change these both messages to Item not found as well ?

Copy link
Member

@garrytrinder garrytrinder May 26, 2020

Choose a reason for hiding this comment

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

Thanks you for your efforts on this @ypcode , however I think we should put a pause on this for the time being rather than trying to code around the issues and avoid unnecessary work.

We reached out to the Graph team about these endpoints and although they are available in the Graph, they appear to be in an alpha state, hence there are no docs.

We expect that there will be changes made before it reaches beta status, therefore we should wait for comms around this release and docs are added to support these endpoints.

@waldekmastykarz waldekmastykarz changed the base branch from dev to master May 31, 2020
@garrytrinder garrytrinder marked this pull request as draft Jun 22, 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 removed their assignment 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

@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)

@ypcode
Copy link
Contributor Author

ypcode commented Aug 23, 2020

@waldekmastykarz @garrytrinder

Okay... I was curious to check, and I did well... 3 months later, this bug we had with different ID is still present !!!

See in images:
image

image

image

IDs are not reliable to fetch a specific item

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 23, 2020

Okay... I was curious to check, and I did well... 3 months later, this bug we had with different ID is still present !!!

Just to check, is the issue, that for the same list you get one ID when you retrieve it, but need to use another one to delete it?

@ypcode
Copy link
Contributor Author

ypcode commented Aug 23, 2020

No no it works "fine" if you pass the right Id...

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Aug 25, 2020

So the PR is good for processing?

@ypcode
Copy link
Contributor Author

ypcode commented Aug 26, 2020

It has everything to work even with the issues from the API :)

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

@waldekmastykarz waldekmastykarz left a comment

Nicely done with a few changed I've done when merging the PR 👍

}

cb();
}, (err: any) => this.handleRejectedODataJsonPromise(err, cmd, cb));
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.

Instead of nesting promises and handling errors twice, we can simplify the code by changing line 64 to return request.delete and then handle the response just once.

`
Remarks:

## Remarks
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.

Markdown content doesn't belong in command's help

!!! attention
This command is based on an API that is currently in preview and is subject to change once the API reached general availability.

Examples:
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 line is indented too far

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