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

feat(groups): add createGroupNotification #311

Merged
merged 5 commits into from
Sep 10, 2018
Merged

Conversation

mjuniper
Copy link
Member

AFFECTS PACKAGES:
@esri/arcgis-rest-groups

@coveralls
Copy link

coveralls commented Sep 10, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 8ac1067 on f/create-notification into 40bc5c1 on master.

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

saweet!

* @param requestOptions - Options for the request
* @returns A Promise that will resolve with the success/failure status of the request
*/
// see http://mediawikidev.esri.com/index.php/ArcGIS.com/User_Notifications
Copy link
Contributor

Choose a reason for hiding this comment

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

lets remove this link since its not on the public web.

users?: string[];
notificationChannelType: string;
clientId?: string;
silentNotification?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we supply a default channel?

notificationChannelType?: "push" | "email" | "builtin"
// ...
notificationChannelType: requestOptions.notificationChannelType || "email"

export interface IGroupNotificationRequestOptions
extends IGroupIdRequestOptions {
subject?: string;
message: string | object;
Copy link
Contributor

Choose a reason for hiding this comment

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

especially since the REST endpoint isn't docced yet, more info in the inline TypeDoc would go a long way.

/*
* subject of the notification. This only applies to email and builtin notifications. For the push notifications, subject/title is provided as a part of the message payload.
*/
subject?: string;

@jgravois jgravois merged commit 6a17fe5 into master Sep 10, 2018
@jgravois jgravois deleted the f/create-notification branch September 10, 2018 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants