Admin notifications #36
Conversation
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
sakeerthy
force-pushed
the
admin-notifications
branch
from
6633edf
to
d8860b4
Compare
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
sakeerthy
force-pushed
the
admin-notifications
branch
from
3db364b
to
328f2cb
Compare
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
| this.notificationCache.push(notification); | ||
| for (let i = 0; i < this.handlers.length; i++) { | ||
| this.handlers[i].handleMessageAdded(); | ||
| _setURL(url: string): void { |
There was a problem hiding this comment.
This should be made more tamper-proof. If you put a var at the top of the file, outside the class, which is false but made true when _setURL is called for the first time, then you can keep url locally here rather than assigning it to this.url, and all future calls to _setURL can be made to fail on purpose due to checking that you already id setURL once.
There was a problem hiding this comment.
Update: please just change this.ws to 'let ws' so it is private.
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
interface/src/mvd-hosting.d.ts
Outdated
| getAll(): Notification[] | void; | ||
| getAllByCategory(type: MVDHosting.NotificationType): Notification[] | void; | ||
| export interface ZoweNotificationManagerInterface { | ||
| push(notification: ZoweNotification): void; |
There was a problem hiding this comment.
get rid of push, updateHandlers, and removeFromCache
Instead, do this
- notify(message): number = you give it a message, and it notifies handlers that the message has been added
- serverNotify(message): promise = this goes to the server, so you dont really get a number, but you do need to know if you succeed or not.
- dismissNotification(id: number) = notifies handlers that the message has been dismissed by the user
- make a removeMessageHandler since you had an addMessageHandler
There was a problem hiding this comment.
Refactor this a bit so that notifications can be local, not touching the server, and that people can choose ot directly touch the notification cache, and that people can be notificed of cache item removal, and that people can unsubscribe entirely rather than just subscribe.
interface/src/mvd-hosting.d.ts
Outdated
| export interface NotificationWatcher { | ||
| handleMessageAdded(): void; | ||
| export interface ZoweNotificationWatcher { | ||
| handleMessageAdded(data: any, index: number): void; |
There was a problem hiding this comment.
Add handleMessageRemoved
| // } | ||
|
|
||
| push(notification: ZoweNotification): void { | ||
| this.ws.send(JSON.stringify(notification)) |
There was a problem hiding this comment.
Change this, there should be no ws.send in this file
…admin-notifications
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
|
|
||
| createNotification(title: string, message: string, type: number, plugin: string, config?: any): ZoweNotification { | ||
| let notification = new ZoweNotification(this.idCount, title, message, type, plugin, config) | ||
| this.idCount = this.idCount + 1; |
There was a problem hiding this comment.
You can allow people to call this function but they should know it does not include an ID because it is meant to be used by notify.
interface/src/mvd-hosting.d.ts
Outdated
| addMessageHandler(object: ZoweNotificationWatcher): void; | ||
| removeMessageHandler(object: ZoweNotificationWatcher): void; | ||
| } | ||
|
|
||
| export interface ZoweNotificationWatcher { | ||
| handleMessageAdded(data: any, index: number): void; |
There was a problem hiding this comment.
This should be "id" since the other one is too. "index" has a similar but different meaning
|
|
||
| constructor(title: string, message: string, type: MVDHosting.ZoweNotificationType, plugin: string, config?: any) { | ||
| constructor(id: number, title: string, message: string, type: MVDHosting.ZoweNotificationType, plugin: string, config?: any) { |
There was a problem hiding this comment.
Need a note that id is "optional" because it will be filled in by the framework later, but at the time the manager broadcasts this object it WILL have an id.
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
Signed-off-by: Suneeth Keerthy <skeerthy@rocketsoftware.com>
Created pull request to see conflicts easier, not ready for review.