Skip to content

#1373: add notes field in order and product types#1597

Merged
kriswest merged 4 commits intofinos:mainfrom
Citi:feature/add-notes-field-to-order-and-product-types
Jun 13, 2025
Merged

#1373: add notes field in order and product types#1597
kriswest merged 4 commits intofinos:mainfrom
Citi:feature/add-notes-field-to-order-and-product-types

Conversation

@Angela-lltt
Copy link
Contributor

@Angela-lltt Angela-lltt commented May 28, 2025

Describe your change

Related Issue

resolves #1373

Contributor License Agreement

  • I acknowledge that a contributor license agreement is required and that I have one in place or will seek to put one in place ASAP.

Review Checklist

  • Issue: If a change was made to the FDC3 Standard, was an issue linked above?
  • CHANGELOG: Is a CHANGELOG.md entry included?
  • API changes: Does this PR include changes to any of the FDC3 APIs (DesktopAgent, Channel, PrivateChannel, Listener, Bridging)?
    • Docs & Sources: If yes, were both documentation (/docs) and sources updated?

      JSDoc comments on interfaces and types should be matched to the main documentation in /docs
    • Conformance tests: If yes, are conformance test definitions (/toolbox/fdc3-conformance) still correct and complete?

      Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined
    • Schemas: If yes, were changes applied to the Bridging and FDC3 for Web protocol schemas?

      The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other
      • If yes, was code generation (npm run build) run and the results checked in?

        Generated code will be found at /src/api/BrowserTypes.ts and/or /src/bridging/BridgingTypes.ts
  • Context types: Were new Context type schemas created or modified in this PR?
    • Were the field type conventions adhered to?
    • Was the BaseContext schema applied via allOf (as it is in existing types)?
    • Was a title and description provided for all properties defined in the schema?
    • Was at least one example provided?
    • Was code generation (npm run build) run and the results checked in?

      Generated code will be found at /src/context/ContextTypes.ts
  • Intents: Were new Intents created in this PR?

@Angela-lltt Angela-lltt requested a review from a team as a code owner May 28, 2025 02:51
@netlify
Copy link

netlify bot commented May 28, 2025

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit f2871cc
🔍 Latest deploy log https://app.netlify.com/projects/fdc3/deploys/6842cecad42dfd0008ef122e
😎 Deploy Preview https://deploy-preview-1597.preview-fdc3.finos.org
📱 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 project configuration.

@robmoffat robmoffat requested a review from a team May 29, 2025 09:13
@robmoffat
Copy link
Member

@Angela-lltt please add a CHANGELOG.md entry.

@finos/fdc3-maintainers and @finos/fdc3-editors please review.

@kriswest
Copy link
Contributor

LGTM - I'll hold on approving until the Changelog is added.

@robmoffat you still need to update the Changelog in this PR which is overlapping: https://github.com/finos/FDC3/pull/1574/files (see comment https://github.com/finos/FDC3/pull/1574/files#r2037646220).

Alternatively, @Angela-lltt could put two changelog entries into her PR and we close yours.

Finally, @Angela-lltt is anything in #1503 that would be of use to your team. We probably don't want to include everything in there, but I'm happy for any of it to be recycled!

@Angela-lltt Angela-lltt reopened this May 30, 2025
Signed-off-by: Angela <lltt_always@163.com>
@Angela-lltt
Copy link
Contributor Author

@robmoffat @kriswest I just updated the CHANGELOG.md, please help to check.
And currently we're using costomized context type in our team, so we're not using items in #1503.

@bingenito
Copy link
Member

bingenito commented Jun 2, 2025

Shouldn't this have generated changes to fdc3-context/generated/context/ContextTypes.ts reflecting the new properties from the schema?

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.

@bingenito is correct. @Angela-lltt could you resolve conflicts with main and then run the following in packages/fdc3-context:
npm run typegen
then add any resulting changes to your PR and it should be good to go.

Also suggested minor tweaks to the changelog entries.

And currently we're using costomized context type in our team, so we're not using items in #1503.

I didn't expect that that you were, as they've not been added to the standard (RFQ type, additional fields on Product, Order and Trade). Another way to look at the question is "Are you using custom types that have fields that we should consider adding to Order, Trade, Product (and a future RFQ type)? Is there overlap between those proposed definitions and custom types that you have to use due to the lack of them in the standard,

CHANGELOG.md Outdated
Comment on lines +10 to +11
* Add a notes field to trade type ([#1563](https://github.com/finos/FDC3/pull/1563))
* Add a notes filed to order and product types ([#1597](https://github.com/finos/FDC3/pull/1597))
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
* Add a notes field to trade type ([#1563](https://github.com/finos/FDC3/pull/1563))
* Add a notes filed to order and product types ([#1597](https://github.com/finos/FDC3/pull/1597))
* Add a notes field to the Trade Context type ([#1563](https://github.com/finos/FDC3/pull/1563))
* Add a notes filed to the Order and Product Context types ([#1597](https://github.com/finos/FDC3/pull/1597))

@kriswest
Copy link
Contributor

kriswest commented Jun 5, 2025

@Lecss and @Angela-lltt could someone run npm run typegen on this branch and push the result? We can complete the review afterwards

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 6, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@Angela-lltt
Copy link
Contributor Author

@kriswest Updated

@kriswest
Copy link
Contributor

kriswest commented Jun 6, 2025

@Angela-lltt thanks! However it looks like your last commit went with the wrong email address (see https://github.com/finos/FDC3/commit/7c978d57b74c799e85207a89232cd3e23c6c87fe.patch and didn't get authorised in Easy CLA.

You can probably fix that with:

git commit --amend --author="Author Name <email@address.com>" --no-edit
git push --force

@kriswest
Copy link
Contributor

kriswest commented Jun 6, 2025

...and there are some conflicts in need of resolving.

CHANGELOG.md Outdated
### Added

* Add a notes field to Trade type ([#1563](https://github.com/finos/FDC3/pull/1563))
* Add a notes filed to Order and Product types ([#1597](https://github.com/finos/FDC3/pull/1597))
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
* Add a notes filed to Order and Product types ([#1597](https://github.com/finos/FDC3/pull/1597))
* Add a notes field to Order and Product types ([#1597](https://github.com/finos/FDC3/pull/1597))

@Angela-lltt Angela-lltt force-pushed the feature/add-notes-field-to-order-and-product-types branch from 7c978d5 to d540ee2 Compare June 6, 2025 11:11
@Angela-lltt Angela-lltt force-pushed the feature/add-notes-field-to-order-and-product-types branch from d540ee2 to ae45ddc Compare June 6, 2025 11:13
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

@bingenito I think all sorted now.

@finos/fdc3-editors pls review.

@Angela-lltt
Copy link
Contributor Author

@kriswest All updated

@kriswest kriswest requested review from a team and bingenito June 6, 2025 11:21
Copy link

@novavi novavi left a comment

Choose a reason for hiding this comment

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

LGTM

@Angela-lltt
Copy link
Contributor Author

Angela-lltt commented Jun 11, 2025

@robmoffat @kriswest it failed on Nodejs CI test check, could you help on it?

@kriswest
Copy link
Contributor

@Angela-lltt you can ignore the failing check - it passed all the tests, but failed to post a comment as the PR came from a fork. The maintainers are going to try and resolve that soon.

@kriswest kriswest changed the title #1373: add notes field in oder and product types #1373: add notes field in order and product types Jun 13, 2025
@kriswest kriswest merged commit b5f970d into finos:main Jun 13, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a notes field to trade, order and product types

5 participants