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

574 StartChat: ChatInitSettings and ChatRoomRef #620

Conversation

bertrand-s
Copy link
Contributor

I made several simplifications to the initial issue (574)

  • ChatInit was renamed ChatInitSettings
  • ChatInitSettings options are not chat-provider specific anymore
  • ChatInitSettings options support a new option (groupRecipients) that allows to create one room per recipients or one room with all members - this is why the proposal is to have StartChat return a list of ChatRoomRef
  • ChatRoomRef app is now an AppMetadata
  • ChatRoomRef id is a simple string

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@kriswest kriswest added enhancement New feature or request context-data FDC3 Context Data Working Group intents Context Data & Intents Contexts & Intents Discussion Group labels Mar 16, 2022
@kriswest kriswest linked an issue Mar 16, 2022 that may be closed by this pull request
| ------------------------------ | --------- | -------- | -------------------------------------------------------------------- |
| `type` | string | Yes | `'fdc3.chat.initSettings'` |
| `chatName` | string | No | `'Instrumet XYZ'` |
| `members` | Contact[] | No | `[contact1, contact2]` |
Copy link
Member

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[].

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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

docs/context/ref/ChatInitSettings.md Outdated Show resolved Hide resolved
docs/context/ref/ChatInitSettings.md Outdated Show resolved Hide resolved
docs/intents/ref/StartChat.md Outdated Show resolved Hide resolved
bertrand-s and others added 4 commits March 31, 2022 15:05
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>
@bertrand-s
Copy link
Contributor Author

As discussed with @rikoe during last meeting, I removed all references to ChatRoomRef, which could be re-introduced when Intent return values are supported

Copy link
Contributor

@mistryvinay mistryvinay left a 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.

@mistryvinay
Copy link
Contributor

@finos/fdc3-maintainers any blockers for not approving?

Copy link
Contributor

@kriswest kriswest left a 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'` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `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.
Copy link
Contributor

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

@kriswest
Copy link
Contributor

kriswest commented May 10, 2022

Needs a CHANGELOG.md entry adding!

@kriswest kriswest added this to the 2.0-candidates milestone May 17, 2022
@mistryvinay mistryvinay changed the base branch from master to context-data-and-intents-consolidated-update-branch May 19, 2022 10:37
@mistryvinay mistryvinay merged commit fef5c5e into finos:context-data-and-intents-consolidated-update-branch May 19, 2022
@@ -0,0 +1,86 @@
---
id: ChatInitSettings
Copy link
Member

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.

@kriswest
Copy link
Contributor

kriswest commented May 19, 2022 via email

@robmoffat
Copy link
Member

robmoffat commented May 19, 2022

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 context data, I still think we should follow the naming scheme we already have for other bits of context: Instrument, Organisation, Contact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Context Data & Intents Contexts & Intents Discussion Group context-data FDC3 Context Data Working Group enhancement New feature or request intents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StartChat and SendChatMessage
6 participants