-
Notifications
You must be signed in to change notification settings - Fork 132
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
592 intent proposal create interaction #747
592 intent proposal create interaction #747
Conversation
✅ Deploy Preview for lambent-kulfi-cf51a7 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@kriswest @mistryvinay the PR build is failing because we reference TimeRange (as discussed at the last working group) and whilst it is documented, the TS definition appears to be missing. I'm happy to add it but I'm assuming it should be in master rather than this branch. |
Hi @pauldyson i think the issue you are seeing is because of this https://github.com/finos/FDC3/pull/724/files#r881622916 will let @kriswest comment if this is correct. |
should this be called "record interaction"? reading through the PR code I got the impression that this was a generic way to start an interaction through various channels. but discussing with the domain experts it's more a case of recording an existing interaction to CRM or similar? |
@Yannick-Symphony I believe it's Create as in CRUD. It's following the naming conventions at https://fdc3.finos.org/docs/next/intents/spec#intent-name-prefixes (although looks like we went off-piste with 'Get' rather than 'Retrieve'. Note that intents also have display names (although there is a slight problem with them as each app defines them #312 - it hasn't been high on the list to solve however so is still open). |
@pauldyson @milindchidrawar could you resolve conflicts - when you push it'll re-run the checks and should pass now. |
docs/context/spec.md
Outdated
- [`fdc3.interaction`](ref/Interaction) ([schema](/schemas/next/interaction.schema.json)) | ||
- [`fdc3.instrument`](ref/Instrument) ([schema](/schemas/next/instrument.schema.json)) | ||
- [`fdc3.instrumentList`](ref/InstrumentList) ([schema](/schemas/next/instrumentList.schema.json)) |
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.
pls move after the fdc3.instrument* types
- [`fdc3.interaction`](ref/Interaction) ([schema](/schemas/next/interaction.schema.json)) | |
- [`fdc3.instrument`](ref/Instrument) ([schema](/schemas/next/instrument.schema.json)) | |
- [`fdc3.instrumentList`](ref/InstrumentList) ([schema](/schemas/next/instrumentList.schema.json)) | |
- [`fdc3.instrument`](ref/Instrument) ([schema](/schemas/next/instrument.schema.json)) | |
- [`fdc3.instrumentList`](ref/InstrumentList) ([schema](/schemas/next/instrumentList.schema.json)) | |
- [`fdc3.interaction`](ref/Interaction) ([schema](/schemas/next/interaction.schema.json)) |
src/context/schemas.json
Outdated
"Interaction": ["https://fdc3.finos.org/schemas/next/interaction.schema.json"], | ||
"Instrument": ["https://fdc3.finos.org/schemas/next/instrument.schema.json"], | ||
"InstrumentList": ["https://fdc3.finos.org/schemas/next/instrumentList.schema.json"], |
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.
Alphabetical order
"Interaction": ["https://fdc3.finos.org/schemas/next/interaction.schema.json"], | |
"Instrument": ["https://fdc3.finos.org/schemas/next/instrument.schema.json"], | |
"InstrumentList": ["https://fdc3.finos.org/schemas/next/instrumentList.schema.json"], | |
"Instrument": ["https://fdc3.finos.org/schemas/next/instrument.schema.json"], | |
"InstrumentList": ["https://fdc3.finos.org/schemas/next/instrumentList.schema.json"], | |
"Interaction": ["https://fdc3.finos.org/schemas/next/interaction.schema.json"], |
src/intents/Intents.ts
Outdated
@@ -13,4 +13,5 @@ export enum Intents { | |||
ViewProfile = 'ViewProfile', | |||
ViewQuote = 'ViewQuote', | |||
ViewResearch = 'ViewResearch', | |||
CreateInteraction = 'CreateInteraction', |
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.
Add in alphabetical order pls
src/intents/standard intents.json
Outdated
}, | ||
{ | ||
"name": "CreateInteraction", | ||
"displayName": "Create Interaction" |
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.
Add in alphabetical order pls
website/sidebars.json
Outdated
@@ -52,6 +52,7 @@ | |||
"intents/ref/ViewProfile", | |||
"intents/ref/ViewQuote", | |||
"intents/ref/ViewResearch", | |||
"intents/ref/CreateInteraction", |
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.
Add in alphabetical order pls
website/sidebars.json
Outdated
"context/ref/Interaction", | ||
"context/ref/Instrument", | ||
"context/ref/InstrumentList", |
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.
Alphabetical order pls
"context/ref/Interaction", | |
"context/ref/Instrument", | |
"context/ref/InstrumentList", | |
"context/ref/Instrument", | |
"context/ref/InstrumentList", | |
"context/ref/Interaction", |
website/yarn.lock
Outdated
@@ -4,34 +4,41 @@ | |||
|
|||
"@ampproject/remapping@^2.1.0": | |||
version "2.2.0" | |||
resolved "https://registry.yarnpkg.com/@ampproject/remapping/-/remapping-2.2.0.tgz#56c133824780de3174aed5ab6834f3026790154d" | |||
resolved "https://registry.npmjs.org/@ampproject/remapping/-/remapping-2.2.0.tgz" |
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.
Minor, but probably best to remove file from PR
Added suggestions for alphabetical order corrections, couldn't do them all unfortunately. |
hi @kriswest |
Hi @kriswest @mistryvinay many thanks for the feedback. I will be working through these updates ahead of our next working group meeting and will let you know if I have any questions. |
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
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.
Theres still a small handful of minor changes to do here - but otherwise this looks good to me!
CHANGELOG.md
Outdated
@@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). | |||
* Added an `interop` field to AppD application records, replacing the `intents` field, to more fully describe an app's use of FDC3 and enable search for apps that 'interoperate' with a selected app ([#697](https://github.com/finos/FDC3/pull/697)) | |||
* Added `AppIdentifier` type, which is a new parent of `AppMetadata` and clarifies required fields for API call parameters which now prefer `appId` and `instanceId` over `name` ([#722](https://github.com/finos/FDC3/pull/722)) | |||
* Added a `getAppMetdata()` function to the desktop agent that can be used to retrieve the full `AppMetadata` for an `AppIdentifier` and reduced types such as `IntentResolution.source` and `ContextMetadata.source` from `AppMetadata` to `AppIdentifier` to clarify what fields a developer can rely on and that they should manually retrieve the full `AppMetadata` when they need it for display purposes. ([#751](https://github.com/finos/FDC3/pull/751)) | |||
* Added `CreateInteraction` intent. To be used when a user wants to record an interaction into a system. New context `Interaction` also introduced. An interaction might be a call, IM, email, a meeting (physical or virtual) or the preparation of some specialist data. ([#747](https://github.com/finos/FDC3/pull/747)) |
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.
Apologies @milindchidrawar this should go into the unreleased section at the top of this file, as 2.0 was already released
docs/context/ref/Interaction.md
Outdated
--- | ||
# `Interaction` | ||
|
||
An interaction is a significant direct exchange of ideas or information between a Sell Side party and one or more Buy Side parties. An interaction might be a call, a meeting (physical or virtual), an IM or the preparation of some specialist data. |
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.
Small edits suggested:
An interaction is a significant direct exchange of ideas or information between a Sell Side party and one or more Buy Side parties. An interaction might be a call, a meeting (physical or virtual), an IM or the preparation of some specialist data. | |
An `Interaction` is a significant direct exchange of ideas or information between a number of participants, e.g. a Sell Side party and one or more Buy Side parties. An `Interaction` might be a call, a meeting (physical or virtual), an IM or the preparation of some specialist data. |
Can you elaborate on how 'preparation of some specialist data' is an interaction?
docs/context/ref/Interaction.md
Outdated
|
||
Notes: | ||
|
||
- interactionType SHOULD be one of 'Instant Message', 'Email', 'Call', or 'Meeting' although other values can be provided |
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.
- interactionType SHOULD be one of 'Instant Message', 'Email', 'Call', or 'Meeting' although other values can be provided | |
- `interactionType` SHOULD be one of `'Instant Message'`, `'Email'`, `'Call'`, or `'Meeting'` although other string values are permitted. |
docs/context/ref/Interaction.md
Outdated
Notes: | ||
|
||
- interactionType SHOULD be one of 'Instant Message', 'Email', 'Call', or 'Meeting' although other values can be provided | ||
- origin indicates the application or service that the interaction was created from to aid in tracing the source of the interaction |
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.
- origin indicates the application or service that the interaction was created from to aid in tracing the source of the interaction | |
- `origin` is used to represent the application or service that the interaction was created from to aid in tracing the source of an interaction. |
--- | ||
# `CreateInteraction` | ||
|
||
Create an interaction with a list of contacts. |
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.
Create an interaction with a list of contacts. | |
Create a record documenting an interaction (calls, meetings, etc.) between a list of contacts. |
src/context/ContextTypes.ts
Outdated
origin?: string; | ||
} | ||
|
||
export interface InstrumentList { |
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.
Copy paste error? This already appears on lines 168-173
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.
Also, this looks like a manual addition as there are other parts that are missing (martialling functions to/from JSON).
That's not the end of the world as we'll re-generate this file after a merge to master. Hence, feel free to either correct this or remove the changes to this file entirely.
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.
Hi @kriswest thanks for your patience with this. This should all be taken care of now.
@milindchidrawar spotted another minor error in the schema - please commit the suggestions I've just added. |
Co-authored-by: Kris West <kris@cosaic.io>
Co-authored-by: Kris West <kris@cosaic.io>
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
I guess that's fine. So there is a single initiator to the interaction, and when we are reporting several messages as part of one interaction, the initiator field cannot be used to track the sender of each message. |
@mistryvinay this PR has conflicts with master again, however, i would suggest just going ahead and starting off a consolidated PR (branch off master), merge this into it (may require @milindchidrawar to merge master and resolve conflicts again, apologies), then we can merge in others (e.g. TransactionResult, Symphony PRs) and submit that consolidated PR to SWG for inclusion. |
35d90e2
into
finos:context-data-and-intents-consolidated-update-branch-2.1
Closes #592