-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat(context): Support Viewing and Adding Related Context to All Asset Types #15453
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: master
Are you sure you want to change the base?
Conversation
| typeWiring -> | ||
| typeWiring | ||
| .dataFetcher("relationships", new EntityRelationshipsResultResolver(graphClient)) | ||
| .dataFetcher( |
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.
REMOVE THIS.
| builder.type( | ||
| "StructuredPropertyEntity", | ||
| typeWiring -> typeWiring.dataFetcher("exists", new EntityExistsResolver(entityService))); | ||
| typeWiring -> |
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.
Remove this
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| * @param userAndGroupUrns List of URNs for the current user and their groups | ||
| * @return The combined filter | ||
| */ | ||
| private Filter buildCombinedFilter(SearchDocumentsInput input, List<String> userAndGroupUrns) { |
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.
Mostly removing and making small tweaks to the filtering logic inside of DocumentSearchFilterUtils.java
| criteria.add( | ||
| CriterionUtils.buildCriterion( | ||
| "parentDocument", Condition.EQUAL, input.getParentDocuments())); | ||
| } else if (input.getParentDocument() != null) { |
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.
No one is using this - we are removing this since we just moved to using parentDocuments instead.
...-graphql-core/src/main/java/com/linkedin/datahub/graphql/types/knowledge/DocumentMapper.java
Show resolved
Hide resolved
|
✅ Meticulous spotted 0 visual differences across 992 screens tested: view results. Meticulous evaluated ~8 hours of user flows against your PR. Expected differences? Click here. Last updated for commit bf16877. This comment will update as new commits are pushed. |
| """ | ||
| Get context documents related to this entity | ||
| """ | ||
| relatedDocuments(input: RelatedDocumentsInput!): RelatedDocumentsResult |
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.
Remove this
| """ | ||
| Get context documents related to this entity | ||
| """ | ||
| relatedDocuments(input: RelatedDocumentsInput!): RelatedDocumentsResult |
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.
remove this for now..
| """ | ||
| Optional parent document URN to filter by (for hierarchical browsing) | ||
| """ | ||
| parentDocument: 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.
Nothing released here, this is safe to change.
| """ | ||
| The starting offset of the result set returned | ||
| """ | ||
| states: [DocumentState!] |
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.
Simplifying this interface a lot based on what we actually need.
Remove states filter - now we always default to PUBLISHED OR UNPUBLISHED (with owner == myself).
Remove includeDrafts - this was premature and is not used yet.
Remove filters - this was premature and is not used yet.
searchFlags - this was not used.
| """ | ||
| Get context documents related to this entity | ||
| """ | ||
| relatedDocuments(input: RelatedDocumentsInput!): RelatedDocumentsResult |
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.
remove - should NOT be on all types.
| input: { | ||
| query: '*', | ||
| parentDocuments: urns, | ||
| states: [DocumentState.Published, DocumentState.Unpublished], |
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.
removing this
| @@ -1,39 +1,13 @@ | |||
| import { useMemo } from 'react'; | |||
|
|
|||
| import { extractMentions } from '@app/document/utils/extractMentions'; | |||
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.
just refactoring :)
| * - Loading children on demand | ||
| */ | ||
|
|
||
| function documentToTreeNode(doc: Document, hasChildren: boolean): DocumentTreeNode { |
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.
just refactoring..
Bundle ReportChanges will increase total bundle size by 63.2kB (0.22%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
| /> | ||
| </Tooltip> | ||
| )} | ||
| <ActionsMenuWrapper> |
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.
This is new!
| border-radius: 4px; | ||
| resize: none; | ||
| overflow: hidden; | ||
| overflow: auto; |
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.
Fixing styling for overflowing titles - now we are wrapping.
| {actor && ( | ||
| <> | ||
| {' by '} | ||
| {(() => { |
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.
Can we simplify this?
| ); | ||
|
|
||
| /** | ||
| * Handle creating a new document |
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.
From the popover
| const location = useLocation(); | ||
| const entityRegistry = useEntityRegistry(); | ||
|
|
||
| const handleDelete = async () => { |
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.
simplifying a lot of this. should be no functional changes..
metadata-service/configuration/src/main/resources/application.yaml
Outdated
Show resolved
Hide resolved
|
|
||
| import { Document, InstitutionalMemoryMetadata } from '@types'; | ||
|
|
||
| export type RelatedItem = |
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.
Needs unit tests
| * Calculates the navigation target after deleting a document. | ||
| * Returns the URN to navigate to, or null if should go to home. | ||
| */ | ||
| export function calculateDeleteNavigationTarget( |
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.
Add unit tests
| const deleteDocument = useCallback( | ||
| async (urn: string) => { | ||
| // Get node for rollback | ||
| // Get node for rollback (may be null if document isn't in tree, e.g., opened in modal) |
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.
Need unit test
| }, [content]); | ||
|
|
||
| return mentions; | ||
| return useMemo(() => extractMentions(content), [content]); |
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.
Unit test
Summary
This is the final PR for UI for context base V1 feature. Specifically this adds:
You can now view related context docs for an asset beside to it's links (for both V1 and V2 summary tab). This includes a new relatedDocuments field on specific entity types. This looks like a lot of changes but we are just adding this to select entity types. Initially I'd added tot he EntityType base interface but that added it to a bunch of things I really didn't want it for (e.g. execution requests, so I decided to target specific entity types for now). I've also added a new EntityCapabilityType in the entity registry that is used to decide whether to fetch and render related docs.
You can now Move / Delete docs from the document profile itself
You can now link related context docs for an asset from the asset profile!
We've made some changes to how searching documents works so that you see PUBLISHED docs OR your own drafts (owned by you) BY DEFAULT. This will be sufficient for all the features that currently use this GraphQL query.
Loom Overview: https://www.loom.com/share/50dbc526c82746978bd163b9526966c7