-
Notifications
You must be signed in to change notification settings - Fork 25
feat(core): align connection note and connection history item IDs… #1289
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
base: feature/VT20-1908-updates-to-creation-and-deletion-of-connections
Are you sure you want to change the base?
Conversation
… in the cloud with new schema
…ection details retrieval and deletion
if (identifier) { | ||
const connectionPair = await this.connectionPairStorage.findById( | ||
`${identifier}:${contactId}` | ||
); |
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 can move this logic to getConnectionShortDetailById
so it's easier to follow. getContactMetadataById
should just return a contact, not a full connection.
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.
And then we can remove the ContactDetailsRecord
type
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 don’t think so. We have the getConnectionShortDetails
function calling this, and it requires the full connection. Also, getConnectionShortDetails
is called from many places
src/ui/components/CredentialDetailModule/CredentialDetailModule.tsx
Outdated
Show resolved
Hide resolved
@@ -313,14 +316,15 @@ class ConnectionService extends AgentService { | |||
contactId: record.id, | |||
...(record.groupId | |||
? { groupId: record.groupId } | |||
: { identifier: record.identifier }), | |||
: { identifier: record.identifier ?? "" }), | |||
}; | |||
} | |||
|
|||
@OnlineOnly | |||
async getConnectionById( | |||
id: string, |
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.
Maybe rename to id
to contactId
everywhere we are passing the contact ID tbh
…nnectionById in IPEX communication service
…re identifier and enhance related tests
…tails in CredentialDetailModule, ConnectionDetails, and Connections components for improved type safety
…tails for improved type safety and consistency
…tions components for enhanced type safety
…parameter across notification and communication services
@@ -109,7 +109,8 @@ const CredentialDetailModule = ({ | |||
await Agent.agent.connections.getConnectionShortDetailById( | |||
connectionId | |||
); | |||
setConnectionShortDetails(shortDetails); | |||
// Credentials are only associated with regular connections | |||
setConnectionShortDetails(shortDetails as RegularConnectionDetails); |
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 use function overloads to tighten things, and remove our usage of as
.
async getConnectionShortDetailById(id: string): Promise<MultisigConnectionDetails>
async getConnectionShortDetailById(id: string, identifier: string): Promise<RegularConnectionDetails>;
async getConnectionShortDetailById(
id: string,
identifier?: string
): Promise<MultisigConnectionDetails | RegularConnectionDetails> {
We might be able to remove ConnectionShortDetails
completely which forces our functions to overload correctly like this.
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
setNotes(connectionDetails.notes || []); | ||
setConnectionHistory(connectionDetails.historyItems || []); | ||
// Since this component only handles regular connections, cast to RegularConnectionDetailsFull | ||
setConnectionDetails(connectionDetails as RegularConnectionDetailsFull); |
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.
Same here I presume
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
@@ -120,7 +120,8 @@ const Connections = () => { | |||
const getConnectionShortDetails = async (connectionId: string) => { | |||
const shortDetails = | |||
await Agent.agent.connections.getConnectionShortDetailById(connectionId); | |||
setConnectionShortDetails(shortDetails); | |||
// Connections page only shows details for regular connections | |||
setConnectionShortDetails(shortDetails as RegularConnectionDetails); |
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.
and here
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
…nd removed the usage of as type assertions in the connection-related code.
…0-1912-align-connection-note-and-history-in-the-clould-with-new-schema
* feat: recovery contacts per identifier * fix: getAllIdentifiers * fix: vuln id * fix: contact.createdAt * fix: date in unit test
Description
Align connection note and connection history item IDs in the cloud with new schema
Checklist before requesting a review
Issue ticket number and link
Testing & Validation