768 Add ViewMessages Intent & SearchCriteria Context #797
Conversation
✅ Deploy Preview for lambent-kulfi-cf51a7 canceled.
|
mistryvinay
added
enhancement
docs/context/ref/SearchCriteria.md
Outdated
| | Property | Type | Required | Example Value | | ||
| |------------------|-----------------|----------|----------------------| | ||
| | `type` | string | Yes | `'fdc3.searchCriteria'` | | ||
| | `contexts` | (Instrument|<br>Contact|<br>Organization|<br>string)[] | Yes | <pre>[<br>  {<br>    "type": "fdc3.instrument",<br>    "id": {<br>      "ticker": "AAPL"<br>    }<br>  },<br>  {<br>    "type": "fdc3.contact",<br>    "name":"Jane Doe",<br>    "id": {<br>      "email": "jane.doe@mail.com"<br>    }<br>  },<br>  {<br>    "type": "fdc3.organization",<br>    "name":"Symphony",<br>  },<br>  "#OrderID45788422",<br>]</pre> | |
There was a problem hiding this comment.
Does this example need to be revisited? removing the html entities.
There was a problem hiding this comment.
Hi @mistryvinay
I removed the HTML entities from the example.
There was a problem hiding this comment.
Actually, the HTML entities are there to make it render on the web. Markdown has some shortcomings when it comes to code examples...
Heres what it looks like in github's preview without the HTML entities:
and with them:
Use the rich preview (the file icon) to see the markdown rendered:
We need to have another go at asking FINOS to adjust the netlify config to ensure it builds previews for PRs like this - right now it only builds if it detects changes in the /website directory...
There was a problem hiding this comment.
OK. @kriswest Do you want me to restore the changes (HTML entities)?
There was a problem hiding this comment.
Managed to track down the preview - still showing state before they were removed:
If you merge master into your PR (needs doing anyway) it should refresh so you can see it without at: https://deploy-preview-797--lambent-kulfi-cf51a7.netlify.app/docs/next/context/ref/searchcriteria (once the deploy has run).
From memory docusaurus really struggles with long code lines in tables. Might be worth a check first in case I'm mis-remembering.
kriswest
changed the title
docs/context/ref/SearchCriteria.md
Outdated
| --- | ||
| # `SearchCriteria` | ||
|
|
||
| A context type representing a search criteria that can be used to filter and display messages from a chat. |
There was a problem hiding this comment.
Given the generic name, type name and structure (an array of context objects), the description should also be generic (but can use chat as an example).
| A context type representing a search criteria that can be used to filter and display messages from a chat. | |
| A context type that represents a simple search criterion, based on a list of other context objects, that can be used to search or filter content in an application (such as messages in a chat app). |
There was a problem hiding this comment.
I'm tempted to also suggest tweaking the name to acknowledge the simplicity of the criterion (a bag of contexts) in case we want to introduce more complex search types in future (but haven't come to a conclusion on that yet - after all we might expand this one with additional fields instead...).
There was a problem hiding this comment.
I also think an explanation of how multiple contexts in the list should be interpreted (to encourage similar behaviour). Should they be combined with AND or OR?
If variation in behaviour is expected or desirable an enumerated field should be added for that (e.g. combinationLogic: "AND") or the contexts field replaced with separate fields (e.g. contexts: { and: [...], or: [...], not: [...]}).
There was a problem hiding this comment.
@kriswest I like your idea about using contexts: { and: [...], or: [...], not: [...]}
It's very generic :)
I'll update the PR accordingly.
docs/context/ref/SearchCriteria.md
Outdated
| | Property | Type | Required | Example Value | | ||
| |------------------|-----------------|----------|----------------------| | ||
| | `type` | string | Yes | `'fdc3.searchCriteria'` | | ||
| | `contexts` | (Instrument|<br>Contact|<br>Organization|<br>string)[] | Yes | `[{ type: "fdc3.instrument", id: { ticker: "AAPL" }}, { type: "fdc3.contact", name: "Jane Doe", id: { email: "jane.doe@mail.com" }}, { type: "fdc3.organization", name: "Symphony" }, "#OrderID45788422"]` | |
There was a problem hiding this comment.
I'd recommend restoring the HTML entities as they make a big difference when rendered (despite looking nuts in the markdown source)
There was a problem hiding this comment.
I'm struggling with the field name as it doesn't accept arbitrary contexts, rather several specific types - one of which isn't actually a context but a string.
Either it needs renaming to criteria or add optional named fields for each type (which allows setting none to reset the filter BTW).
Do you expect more than one contact or instrument to be added? If so those separate fields would also be arrays, otherwise single, optional objects.
Do you think this might extend to additional types in future. That is more likely if the type is generic and used beyond chat. In which case contexts is a valid name, if the string criteria is moved to a separate field.
There was a problem hiding this comment.
I'll rename 'contexts' into 'criteria'.
@kriswest I don't get your point: 'If so those separate fields would also be arrays'
I have in mind to use something like that in case of multiple Contact, Instrument:
criteria: [
{
type: "fdc3.contact",
name: "Jane Doe",
id: {
email: "jane.doe@mail.com"
}
},
{
type: "fdc3.contact",
name: "Jean Dupont",
id: {
email: "jean.dupond@mail.com"
}
},
{
type: "fdc3.organization",
name: "Symphony"
},
{
type: "fdc3.instrument",
id: {
ticker: "AAPL"
}
},
{
type: "fdc3.instrument",
id: {
ticker: "ABC"
}
}
]So, there is only 1 array accepting any Contact, Intents, et ....
Of course, what is missing here is the AND/OR/NOT criteria ;)
There was a problem hiding this comment.
Do you expect more than one contact or instrument to be added? If so those separate fields would also be arrays, otherwise single, optional objects.
What I meant was, if you separated out the 'contexts' into separate fields for each type, would you be accept just a single contact or an array of contacts.
There was a problem hiding this comment.
I have in mind to use only one array (criteria) and not one field/array per type.
This array could contain objects of different types (like Contact and Organization and Instrument...)
@kriswest Do you agree with this solution? Or would you prefer an array by type?
docs/intents/ref/ViewMessages.md
Outdated
| --- | ||
| # `ViewMessages` | ||
|
|
||
| Search and display a list of messages to the user. |
There was a problem hiding this comment.
| Search and display a list of messages to the user. | |
| Search and display a list of messages (for example in a chat application or CRM) to the user. |
There was a problem hiding this comment.
Thanks. I updated the PR.
docs/intents/ref/ViewMessages.md
Outdated
|
|
||
| ## Example | ||
|
|
||
| Request to display messages for a specific ticker: |
There was a problem hiding this comment.
| Request to display messages for a specific ticker: | |
| Request display of messages relating to a specific `fdc3.instrument` (representing a ticker): |
There was a problem hiding this comment.
Thanks. I updated the PR.
docs/intents/ref/ViewMessages.md
Outdated
| fdc3.raiseIntent('ViewMessages', searchCriteria); | ||
| ``` | ||
|
|
||
| Request to display messages for a specific **contact**: |
There was a problem hiding this comment.
| Request to display messages for a specific **contact**: | |
| Request display of messages relating to a specific `fdc3.contact`: |
There was a problem hiding this comment.
Thanks. I updated the PR.
docs/intents/ref/ViewMessages.md
Outdated
| fdc3.raiseIntent('ViewMessages', searchCriteria); | ||
| ``` | ||
|
|
||
| Request to display messages for a specific **organization**: |
There was a problem hiding this comment.
Probably redundant. It'd be more succinct to add a line of copy either at the start of the example section or under possible contexts explaining that any appropriate contexts (or combination of contexts!) can be added to the search criteria.
docs/intents/ref/ViewMessages.md
Outdated
| fdc3.raiseIntent('ViewMessages', searchCriteria); | ||
| ``` | ||
|
|
||
| Request to display messages with **multiples** criteria: |
There was a problem hiding this comment.
| Request to display messages with **multiples** criteria: | |
| Request display of messages matching _multiple_ criteria: |
Again, we need an indication of how the multiple criteria are expected to be combined.
docs/intents/ref/ViewMessages.md
Outdated
| * [Instrument](../../context/ref/Instrument) | ||
| * [Contact](../../context/ref/Contact) | ||
| * [Organization](../../context/ref/Organization) | ||
| * [SearchCriteria](../../context/ref/SearchCriteria) |
There was a problem hiding this comment.
| * [Instrument](../../context/ref/Instrument) | |
| * [Contact](../../context/ref/Contact) | |
| * [Organization](../../context/ref/Organization) | |
| * [SearchCriteria](../../context/ref/SearchCriteria) | |
| * [SearchCriteria](../../context/ref/SearchCriteria) | |
| * [Instrument](../../context/ref/Instrument) | |
| * [Contact](../../context/ref/Contact) | |
| * [Organization](../../context/ref/Organization) |
Might as well promote SearchCriteria to the head of the list as its of most interest here
There was a problem hiding this comment.
Ok I updated the order.
| @@ -8,10 +8,11 @@ export enum ContextTypes { | |||
| Email = 'fdc3.email', | |||
| Instrument = 'fdc3.instrument', | |||
| InstrumentList = 'fdc3.instrumentList', | |||
| Nothing = 'fdc3.nothing', | |||
There was a problem hiding this comment.
Thanks :)
|
Hi @kriswest |
I haven't yet had a chance for a full review, but as the query fields are restricted so is the use-case. Hence, I still think the type should be scoped for chat. I.e. fdc3.chat.searchCriteria |
|
@kriswest You're right, I renamed |
|
Including notes from our last meeting (Sep 2022). With recommended changes for this PR.
|
symphony-jean-michael
force-pushed
the
feature/ViewMessages
branch
from
37b5f77 to
2b9931a
Compare
|
@mistryvinay FYI:
|
docs/context/ref/SearchCriteria.md
Outdated
| --- | ||
| # `SearchCriteria` | ||
|
|
||
| A context type that represents a simple search criterion, based on a list of other context objects, that can be used to search or filter content in an application (such as messages in a chat app). |
There was a problem hiding this comment.
| A context type that represents a simple search criterion, based on a list of other context objects, that can be used to search or filter content in an application (such as messages in a chat app). | |
| A context type that represents a simple search criterion, based on a list of other context objects, that can be used to search or filter messages in a chat application. |
Change description as we're only covering that use case now
docs/context/ref/SearchCriteria.md
Outdated
| @@ -0,0 +1,64 @@ | |||
| --- | |||
| id: SearchCriteria | |||
There was a problem hiding this comment.
We should probably rename this ChatMessageSearchCriteria or ChatSearchCriteria as this doesn't cover all types of search and something in the future might.
docs/context/ref/SearchCriteria.md
Outdated
| | Property | Type | Required | Example Value | | ||
| |------------------|-----------------|----------|----------------------| | ||
| | `type` | string | Yes | `'fdc3.chat.searchCriteria'` | | ||
| | `criteria` | (Instrument|<br>Contact|<br>Organization|<br>string)[] | Yes | <pre>[<br>  {<br>    "type": "fdc3.instrument",<br>    "id": {<br>      "ticker": "AAPL"<br>    }<br>  },<br>  {<br>    "type": "fdc3.contact",<br>    "name":"Jane Doe",<br>    "id": {<br>      "email": "jane.doe@mail.com"<br>    }<br>  },<br>  {<br>    "type": "fdc3.organization",<br>    "name":"Symphony",<br>  },<br>  "#OrderID45788422",<br>]</pre> | |
There was a problem hiding this comment.
| | `criteria` | (Instrument|<br>Contact|<br>Organization|<br>string)[] | Yes | <pre>[<br>  {<br>    "type": "fdc3.instrument",<br>    "id": {<br>      "ticker": "AAPL"<br>    }<br>  },<br>  {<br>    "type": "fdc3.contact",<br>    "name":"Jane Doe",<br>    "id": {<br>      "email": "jane.doe@mail.com"<br>    }<br>  },<br>  {<br>    "type": "fdc3.organization",<br>    "name":"Symphony",<br>  },<br>  "#OrderID45788422",<br>]</pre> | | |
| | `criteria` | (Instrument |<br>Contact |<br>Organization |<br>string)[] | Yes | <pre>[<br>  {<br>    "type": "fdc3.instrument",<br>    "id": {<br>      "ticker": "AAPL"<br>    }<br>  },<br>  {<br>    "type": "fdc3.contact",<br>    "name":"Jane Doe",<br>    "id": {<br>      "email": "jane.doe@mail.com"<br>    }<br>  },<br>  {<br>    "type": "fdc3.organization",<br>    "name":"Symphony",<br>  },<br>  "#OrderID45788422",<br>]</pre> | |
docs/context/spec.md
Outdated
| @@ -159,6 +159,7 @@ The following are standard FDC3 context types: | |||
|
|
|||
| * [`fdc3.chart`](ref/Chart) ([schema](/schemas/next/chart.schema.json)) | |||
| * [`fdc3.chat.initSettings`](ref/ChatInitSettings) ([schema](/schemas/next/chatInitSettings.schema.json)) | |||
| * [`fdc3.chat.searchCriteria`](ref/SearchCriteria) ([schema](/schemas/next/searchCriteria.schema.json)) | |||
src/context/ContextType.ts
Outdated
| Organization = 'fdc3.organization', | ||
| Portfolio = 'fdc3.portfolio', | ||
| Position = 'fdc3.position', | ||
| Nothing = 'fdc3.nothing', | ||
| SearchCriteria = 'fdc3.chat.searchCriteria', |
src/context/ContextTypes.ts
Outdated
| @@ -187,6 +187,11 @@ export interface Position { | |||
| name?: string; | |||
| } | |||
|
|
|||
| export interface SearchCriteria { | |||
src/context/schemas.json
Outdated
| @@ -13,6 +13,7 @@ | |||
| "Organization": ["https://fdc3.finos.org/schemas/next/organization.schema.json"], | |||
| "Portfolio": ["https://fdc3.finos.org/schemas/next/portfolio.schema.json"], | |||
| "Position": ["https://fdc3.finos.org/schemas/next/position.schema.json"], | |||
| "SearchCriteria": ["https://fdc3.finos.org/schemas/next/searchCriteria.schema.json"], | |||
| } | ||
| } | ||
| }, | ||
| "required": ["type", "criteria"] |
There was a problem hiding this comment.
| "required": ["type", "criteria"] | |
| "required": ["criteria"] |
type is already required by context schema
website/sidebars.json
Outdated
| @@ -73,6 +74,7 @@ | |||
| "context/ref/Organization", | |||
| "context/ref/Portfolio", | |||
| "context/ref/Position", | |||
| "context/ref/SearchCriteria", | |||
| } | ||
| } | ||
| }, | ||
| "required": ["type", "criteria"] |
There was a problem hiding this comment.
| "required": ["type", "criteria"] | |
| "required": ["criteria"] |
Already required by context schema
|
Hi @kriswest |
|
FYI, this PR is included in new one #882 |
This PR adds the 'ViewMessages' intent.
See initial discussion #768