Skip to content

#1373: add a notes field on fdc3.trade#1563

Merged
robmoffat merged 1 commit intofinos:mainfrom
Citi:feature/add-notes-field-to-fdc3.trade
Apr 10, 2025
Merged

#1373: add a notes field on fdc3.trade#1563
robmoffat merged 1 commit intofinos:mainfrom
Citi:feature/add-notes-field-to-fdc3.trade

Conversation

@Lecss
Copy link
Copy Markdown
Contributor

@Lecss Lecss commented Apr 4, 2025

Describe your change

Related Issue

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?

@Lecss Lecss requested a review from a team as a code owner April 4, 2025 09:14
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Apr 4, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Angela-lltt / name: Angela (57be2f3)

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 4, 2025

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit 57be2f3
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/67efa2e60acb5300083841ff
😎 Deploy Preview https://deploy-preview-1563--fdc3.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 configuration.

@robmoffat robmoffat requested a review from mistryvinay April 4, 2025 09:15
@kriswest
Copy link
Copy Markdown
Contributor

kriswest commented Apr 7, 2025

Could do with a description, linked issue and Change log entry adding to PR. Otherwise, LGTM.

Copy link
Copy Markdown
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. :)

@robmoffat robmoffat merged commit 9c5ea8f into finos:main Apr 10, 2025
10 of 11 checks passed
@kriswest
Copy link
Copy Markdown
Contributor

This still doesn't have a description, linked issue, Changelog entry or a review by an FDC3 editor - why have you merged it @robmoffat ?

@kriswest
Copy link
Copy Markdown
Contributor

@Lecss This needs a post-hoc review by an editor and a subsequent PR raising with a changelog.

@mistryvinay please do run through the review checklist before approving. @robmoffat please don't merge PRs with changes to the Standard without editorial reivew or a complete review checklist.

@robmoffat
Copy link
Copy Markdown
Member

@robmoffat please don't merge PRs with changes to the Standard without editorial reivew or a complete review checklist.

sorry, you're right. I saw Vinay approved it and thought it was good to go

@mistryvinay
Copy link
Copy Markdown
Contributor

@robmoffat @kriswest apologies - my bad for missing Kris comments re changelog.

@kriswest
Copy link
Copy Markdown
Contributor

No worries - no harm done, other than someone needs to raise PR for a CHANGELOG (unless there is anything Vinay and I missed in the review).

Its a pity our branch protection doesn't enforce the editor's review. It probably could if we set them up as codeowners of website/docs as that should catch most changes that apply to the standard and miss out those that are just software changes that only need maintainer approval.

@kriswest
Copy link
Copy Markdown
Contributor

kriswest commented Apr 10, 2025

PR doesn't actually fully address the issue it relates to as it doesn't update the Order and Product schemas!

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.

4 participants