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

592 771 context proposal TransactionResult #761

Conversation

milindchidrawar
Copy link
Contributor

Suggested implementation of transaction result as proposed in context data intents working group #742

@netlify
Copy link

netlify bot commented Jun 22, 2022

Deploy Preview for lambent-kulfi-cf51a7 ready!

Name Link
🔨 Latest commit 9d8d14a
🔍 Latest deploy log https://app.netlify.com/sites/lambent-kulfi-cf51a7/deploys/637c74ce003348000937e80a
😎 Deploy Preview https://deploy-preview-761--lambent-kulfi-cf51a7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mistryvinay mistryvinay added enhancement New feature or request Context Data & Intents Contexts & Intents Discussion Group labels Jul 1, 2022
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.

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/context/ContextType.ts Show resolved Hide resolved
@@ -13,4 +13,5 @@ export enum Intents {
ViewProfile = 'ViewProfile',
ViewQuote = 'ViewQuote',
ViewResearch = 'ViewResearch',
CreateInteraction = 'CreateInteraction',
Copy link
Contributor

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

Copy link
Contributor Author

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"]
Copy link
Contributor

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.

Copy link
Contributor Author

@milindchidrawar milindchidrawar Jul 6, 2022

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"]
}

Copy link
Contributor

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.

Copy link
Contributor

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 |
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

src/context/ContextTypes.ts Outdated Show resolved Hide resolved
---
# `TransactionResult`

The result of any given create, update or delete intent.
Copy link
Contributor

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

Copy link
Contributor Author

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

@mistryvinay
Copy link
Contributor

mistryvinay commented Jul 4, 2022

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:

  • Explanation of the use case/ problem solved by this PR
  • Documentation in both the Context and Intent parts of the Standard describing how to use it

I'm including the issue raised by @pauldyson CreateInteraction proposal as a reference if it helps #592

@mistryvinay mistryvinay linked an issue Jul 6, 2022 that may be closed by this pull request
@milindchidrawar
Copy link
Contributor Author

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.

@kriswest kriswest changed the title 592 intent proposal create transaction result 592 771 context proposal TransactionResult Jul 6, 2022
@kriswest
Copy link
Contributor

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

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

Suggested change
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.

Comment on lines 23 to 25
| `type` | Context | Yes | See Below |
| `status` | string | Yes | `'Updated'` |
| `context` | string | Yes | See Below |
Copy link
Contributor

@kriswest kriswest Jul 21, 2022

Choose a reason for hiding this comment

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

Suggested change
| `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 |

Comment on lines 149 to 151
- [`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))
Copy link
Contributor

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

Suggested change
- [`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))

Copy link
Contributor

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

Comment on lines 12 to 14
"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"],
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
"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"],

Comment on lines 58 to 61
},
{
"name": "CreateInteraction",
"displayName": "Create Interaction"
Copy link
Contributor

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

@@ -52,6 +52,7 @@
"intents/ref/ViewProfile",
"intents/ref/ViewQuote",
"intents/ref/ViewResearch",
"intents/ref/CreateInteraction",
Copy link
Contributor

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

@@ -69,13 +70,15 @@
"context/ref/Country",
"context/ref/Currency",
"context/ref/Email",
"context/ref/Interaction",
Copy link
Contributor

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))
Copy link
Contributor

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

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.

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.

docs/intents/ref/CreateInteraction.md Outdated Show resolved Hide resolved
docs/context/ref/Interaction.md Outdated Show resolved Hide resolved
docs/context/ref/Interaction.md Show resolved Hide resolved
docs/context/ref/Interaction.md Outdated Show resolved Hide resolved
docs/intents/ref/CreateInteraction.md Outdated Show resolved Hide resolved
docs/intents/ref/CreateInteraction.md Outdated Show resolved Hide resolved
src/context/schemas/interaction.schema.json Outdated Show resolved Hide resolved
@@ -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"
Copy link
Contributor

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

milindchidrawar and others added 9 commits November 22, 2022 06:44
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>
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.

LGTM

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.

Should be fine after consolidation with the CreateInteraction PR

@mistryvinay mistryvinay changed the base branch from master to context-data-and-intents-consolidated-update-branch-2.1 March 22, 2023 13:18
@mistryvinay mistryvinay merged commit 805d4b2 into finos:context-data-and-intents-consolidated-update-branch-2.1 Mar 22, 2023
@kriswest kriswest modified the milestones: 2.1-candidates, 2.1 Apr 27, 2023
@kriswest kriswest removed this from the 2.1 milestone Jul 25, 2023
@bingenito bingenito mentioned this pull request Nov 6, 2023
18 tasks
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context Proposal: TransactionResult
4 participants