-
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 771 context proposal TransactionResult #761
592 771 context proposal TransactionResult #761
Conversation
✅ Deploy Preview for lambent-kulfi-cf51a7 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
This is a great start. It'll need some additional documentation under both the Intents and Context overviews and a reference from the API overview I think.
I look forward to discussing it at the next meeting
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.
Please add in alphabetical order
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.
Updated.
"anyOf": [ | ||
{ | ||
"type": "string", | ||
"enum": ["Created", "Deleted", "Updated"] |
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.
I think this needs to include error statuses - unless we decide that the raised intent itself should return an error.
I think we should aim to define the statuses, rather than allow any string (as the anyOf is currently allowing).
I would also suggest adding a string message field - particularly for error statuses.
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 the feedback
I see where you're coming from but I'm not sure how best to update the json. Would this do the trick?
{
"$schema": "http://json-schema.org/draft-07/schema#",
"$id": "https://fdc3.finos.org/schemas/next/transactionresult.schema.json",
"type": "object",
"title": "TransactionResult",
"allOf": [{ "$ref": "context.schema.json#" }],
"properties": {
"type": { "const": "fdc3.transactionResult" },
"status": {
"type": "string",
"enum": ["Created", "Deleted", "Updated", "Failed"]
},
"context": { "$ref": "context.schema.json#" }
},
"message": {
"type": "string"
},
"required": ["status", "context"]
}
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, LGTM, although context
could be optional I suppose... I guess the question is: do we want to force something to be returned?
I'm also wondering about the name of that field. Perhaps it should more strongly imply a particular use (e.g. result
or output
). Just a thought at this stage.
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.
I think we probably do want something to come back. If they really don't want to return something I suppose they can put fdc3.nothing
in ;-)
|-------------|---------|----------|-------------------| | ||
| `type` | string | Yes | `'fdc3.transactionResult'` | | ||
| `status` | string | Yes | `'Updated'` | | ||
| `context` | string | Yes | See Below | |
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.
Type should be Context, which will allow any other FDC3 context to be wrapped
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.
This has been updated now
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.
Not seeing the change... however the type filed is now Context... will add a suggestion to correct both
--- | ||
# `TransactionResult` | ||
|
||
The result of any given create, update or delete intent. |
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.
Lets discuss this description (and other docs required) at next meeting
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.
Neither @pauldyson or I are available to join tomorrow's context and intents discussion meeting I'm afraid.
I'm out on annual leave from tomorrow but if you could share your feedback on the description, I will take a look on my return (22nd July). Thanks
Hi @milindchidrawar forgot to mention congrats on your first PR to FDC3! So we can track this PR correctly, could we open a related issue that includes the following:
I'm including the issue raised by @pauldyson |
Hi @kriswest @mistryvinay many thanks for the feedback. This is very helpful I've just added another commit that should hopefully address the points you've raised. I have left a few questions in response to your feedback and would appreciate your thoughts. @pauldyson and I are unable to attend the next two context and intents meetings, assuming the next one is two weeks today. I will review your feedback when I return on 22nd July. Thanks Milind. |
PR is based off of #747 and should not be merged until that has been, unless it is rebased on master first. |
--- | ||
# `TransactionResult` | ||
|
||
The result of any given create, update or delete intent. |
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.
Heres a start on a deeper description. We'll also need to add something to the Intents overview doc describing the same. I'd suggest adding a new section at the bottom of https://fdc3.finos.org/docs/next/intents/spec for now, called something like:
Returning a status For transactional Intents
The result of any given create, update or delete intent. | |
`TransactionResult` is intended to be returned as an [`IntentResult`](../../api/Types#intentresult) by intents that create, retrieve, update or delete content or records in another application. Its purpose is to provide a status and message (where needed) for the transaction and MAY wrap a returned context object. |
| `type` | Context | Yes | See Below | | ||
| `status` | string | Yes | `'Updated'` | | ||
| `context` | string | Yes | See Below | |
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.
| `type` | Context | Yes | See Below | | |
| `status` | string | Yes | `'Updated'` | | |
| `context` | string | Yes | See Below | | |
| `type` | string | Yes | 'fdc3.transactionResult' | | |
| `status` | string | Yes | `'Updated'` | | |
| `context` | Context | Yes | See Below | |
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)) |
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.
or rather do so in PR #747 - added suggestion there
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.
"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/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.
Alphabetical order pls - also added to #747
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.
Alphabetical order pls - also added to #747
website/sidebars.json
Outdated
@@ -69,13 +70,15 @@ | |||
"context/ref/Country", | |||
"context/ref/Currency", | |||
"context/ref/Email", | |||
"context/ref/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.
Alphabetical order pls - also added to #747
CHANGELOG.md
Outdated
@@ -7,6 +7,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). | |||
## [Unreleased] | |||
|
|||
### Added | |||
* 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.
Need a change log specific to this PR - this relates to #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.
A few corrections to make (mostly with suggestions you can just commit). As some are from #747 I would apply them there then merge into this PR branch and take care of the remaining items. In particular, this PR needs its own changelog entry.
Otherwise its just about there.
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.
Probably best to remove the lockfile from the PR....
Co-authored-by: Kris West <kris@cosaic.io>
Co-authored-by: Kris West <kris@cosaic.io>
Co-authored-by: Kris West <kris@cosaic.io>
Co-authored-by: Kris West <kris@cosaic.io>
Co-authored-by: Kris West <kris@cosaic.io>
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
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.
Should be fine after consolidation with the CreateInteraction PR
805d4b2
into
finos:context-data-and-intents-consolidated-update-branch-2.1
Suggested implementation of transaction result as proposed in context data intents working group #742