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 graph subscription add command #1293

Closed
wants to merge 1 commit into from
Closed

Adds graph subscription add command #1293

wants to merge 1 commit into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented Dec 27, 2019

PR for issue #1100

Adds graph subscription add command

@ypcode ypcode changed the title Adds graph subscription add command #1100 Adds graph subscription add command Dec 27, 2019
@coveralls
Copy link

coveralls commented Dec 27, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 2d438fc on ypcode:PR/graph-subscriptions-add into 520f164 on pnp:dev.

@garrytrinder
Copy link
Member

garrytrinder commented Jan 1, 2020

Thank you @ypcode we will review shortly 👍🏻

@waldekmastykarz waldekmastykarz self-assigned this Jan 26, 2020
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Hey @ypcode, could you please adjust a couple of things before we merge it? Thanks! 👍

src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscriptions/subscription-add.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@VelinGeorgiev VelinGeorgiev left a comment

@ypcode , thanks for this PR, nicely done! I have left some comments. Could you please have a look and let me know what do you think?

src/Utils.ts Outdated Show resolved Hide resolved
src/Utils.ts Outdated Show resolved Hide resolved
src/Utils.ts Outdated Show resolved Hide resolved
src/Utils.ts Show resolved Hide resolved
src/o365/graph/commands/subscription/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscription/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscription/subscription-add.ts Outdated Show resolved Hide resolved
src/o365/graph/commands/subscription/subscription-add.ts Outdated Show resolved Hide resolved
@waldekmastykarz
Copy link
Member

waldekmastykarz commented Feb 15, 2020

Hey @ypcode are you still working on this PR?

@ypcode
Copy link
Contributor Author

ypcode commented Feb 22, 2020

Hi @waldekmastykarz , sorry for the long delay... will see to address the requested changes right now :)

@ypcode ypcode requested a review from VelinGeorgiev Feb 23, 2020
@waldekmastykarz
Copy link
Member

waldekmastykarz commented Feb 23, 2020

No problem @ypcode. Thank you! 👏

@VelinGeorgiev
Copy link
Contributor

VelinGeorgiev commented Mar 20, 2020

@waldekmastykarz , I think this is ready for review. Will remove the work in progress label now. @ypcode please let us know if some changes has to still be made from your side.

@ypcode
Copy link
Contributor Author

ypcode commented Mar 20, 2020

@waldekmastykarz waldekmastykarz self-assigned this Mar 21, 2020
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Nicely done with a few things I've fixed along the road when merging the PR 👍

return 'Required option notificationUrl is missing';
}

if (args.options.notificationUrl.indexOf('https://') != 0) {
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 22, 2020

Choose a reason for hiding this comment

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

We should use !== instead

}

if (args.options.notificationUrl.indexOf('https://') != 0) {
return `The specified notification URL '${args.options.notificationUrl}' is not a valid Webhook endpoint URL`;
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 22, 2020

Choose a reason for hiding this comment

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

It would be clearer to say that the notification URL must start with https:// to make it easier for the user to correct the error. Otherwise it's hard for them to guess what constitutes a valid URL

}

if (!this.isValidChangeTypes(args.options.changeType)) {
return 'The specified changeType is invalid';
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 22, 2020

Choose a reason for hiding this comment

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

Let's add valid options to help the user recover from the error

## Remarks

On personal OneDrive, you can subscribe to the root folder or any subfolder in that drive.
On OneDrive for Business, you can subscribe to only the root folder.
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 22, 2020

Choose a reason for hiding this comment

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

In md we shouldn't break lines ourselves and instead let the text overflow automatically.

@@ -80,7 +80,8 @@ nav:
- schemaextension add: 'cmd/graph/schemaextension/schemaextension-add.md'
- schemaextension get: 'cmd/graph/schemaextension/schemaextension-get.md'
- schemaextension remove: 'cmd/graph/schemaextension/schemaextension-remove.md'
- schemaextension set: 'cmd/graph/schemaextension/schemaextension-set.md'
- subscription:
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 22, 2020

Choose a reason for hiding this comment

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

I believe you accidentally removed the schemaextension set entry

`-u, --notificationUrl <notificationUrl>`|The URL of the endpoint that will receive the notifications. This URL must use the HTTPS protocol
`-e, --expirationDateTime [expirationDateTime]`|The date and time when the webhook subscription expires. The time is in UTC, and can be an amount of time from subscription creation that varies for the resource subscribed to. If not specified, the maximum allowed expiration for the specified resource will be used
`-s, --clientState [clientState]`|The value of the clientState property sent by the service in each notification. The maximum length is 128 characters
`-o, --output [output]`|Output type. `json|text`. Default `text`
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 22, 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 query and pretty options

@waldekmastykarz waldekmastykarz removed their assignment Mar 22, 2020
@waldekmastykarz
Copy link
Member

waldekmastykarz commented Mar 22, 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.

None yet

5 participants