-
Notifications
You must be signed in to change notification settings - Fork 129
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
574 StartChat: ChatInitSettings and ChatRoomRef #620
574 StartChat: ChatInitSettings and ChatRoomRef #620
Conversation
docs/context/ref/ChatInitSettings.md
Outdated
| ------------------------------ | --------- | -------- | -------------------------------------------------------------------- | | ||
| `type` | string | Yes | `'fdc3.chat.initSettings'` | | ||
| `chatName` | string | No | `'Instrumet XYZ'` | | ||
| `members` | Contact[] | No | `[contact1, contact2]` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could use ContactList
instead of Contact[]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I though of this option, but I realised that ContactList adds extra verbosity (type, sub-structure, etc.) this is why I preferred the Contact[] option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bertrand-s we're due to have a conversation at some point about the list types and whether there should be an easier / generic way to create them so they don't have to be explicit type (has been raised in meetings a few times). I'd prefer []
to List
to be added to the base type name (but that's another conversation).
The APIs need a context type currently and can't take an array of them. Although this is embedded inside another type, I think it might be confusing / awkward in some situations for us to have a mixed convention on this (e.g. if the members were pulled out to send on to a CRM in another intent and then had to be converted into a ContactList).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - I will update the PR with Contact and ContactList then
Co-authored-by: Hugh Troeger <troeger.hugh@gmail.com>
Co-authored-by: Hugh Troeger <troeger.hugh@gmail.com>
Co-authored-by: Hugh Troeger <troeger.hugh@gmail.com>
As discussed with @rikoe during last meeting, I removed all references to ChatRoomRef, which could be re-introduced when Intent return values are supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@finos/fdc3-maintainers any blockers for not approving? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| Property | Type | Required | Example Value | | ||
| ------------------------------ | ----------- | -------- | -------------------------------------------------------------------- | | ||
| `type` | string | Yes | `'fdc3.chat.initSettings'` | | ||
| `chatName` | string | No | `'Instrumet XYZ'` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `chatName` | string | No | `'Instrumet XYZ'` | | |
| `chatName` | string | No | `'Instrument XYZ'` | |
@@ -80,6 +80,13 @@ The identifier "foo" is proprietary, an application that can use it is free to d | |||
The following are standard FDC3 context types. | |||
__Note:__ The specification for these types are shared with the [FINOS Financial Objects](https://fo.finos.org) definitions, JSON schemas are hosted with FDC3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for @greyseer256: this sentence is no longer correct once we merge in new types - and that group has changed focus/no longer maintains those definitions. Hence, its probably time to drop this
Needs a CHANGELOG.md entry adding! |
fef5c5e
into
finos:context-data-and-intents-consolidated-update-branch
@@ -0,0 +1,86 @@ | |||
--- | |||
id: ChatInitSettings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest calling this ChatSettings? It seems like these settings would be relevant even after the chat had been created, and could be the basis for further, ongoing intents. e.g. EmailTranscript, CreateAppointment, RecordInCRM, etc. which could be handled by context listeners.
As discussed in the meetings, it is expected that a ChatRoomRef type will
be added later and returned by intents that create a chat and it's that
type that would be used for those purposes @robmoffat.
I think there's more on this in meeting minutes.
…On Thu, 19 May 2022, 13:23 Rob Moffat, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In docs/context/ref/ChatInitSettings.md
<#620 (comment)>:
> @@ -0,0 +1,86 @@
+---
+id: ChatInitSettings
Suggest calling this ChatSettings? It seems like these settings would be
relevant even after the chat had been created, and could be the basis for
further, ongoing intents. e.g. EmailTranscript, CreateAppointment,
RecordInCRM, etc. which could be handled by context listeners.
—
Reply to this email directly, view it on GitHub
<#620 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM7PBB3M4DSGXIU7A44CNLVKYXEPANCNFSM5QGPDALA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Yes, that's very symphony-specific: there, rooms have names, chats are between ad-hoc groups of people. I mean, there's an argument that there should be a single context object covering both but I expect somewhere you've already had that discussion before... But, based on the idea that this should be |
I made several simplifications to the initial issue (574)