-
Notifications
You must be signed in to change notification settings - Fork 38
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
[Datahub] Feature catalog - Load attributes description from a linked record if present #1137
base: main
Are you sure you want to change the base?
Conversation
Affected libs: Affected apps:
|
0451715
to
e54e85e
Compare
📷 Screenshots are here! |
97caf72
to
6909b57
Compare
@@ -28,6 +29,21 @@ export const loadFullMetadataFailure = createAction( | |||
props<{ otherError?: string; notFound?: boolean }>() | |||
) | |||
|
|||
export const loadFeatureCatalog = createAction( |
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.
@jahow says : need to be tested (not action but effects, facade, reducer .. )
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.
test on action done previously
@@ -2642,6 +2643,7 @@ describe('Gn4Converter', () => { | |||
extras: { | |||
catalogUuid: 'metawal.wallonie.be', | |||
favoriteCount: 0, | |||
featureTypes: [], |
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.
@jahow says : is there a way to get a test to show that this takes in the correct value? e.g. by changing the mock values used as input?
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.
If i understand well we tested this in the gn4 repository by checking featureType has some elements no ?
d9bdc0a
to
f98e22f
Compare
@@ -2642,6 +2643,7 @@ describe('Gn4Converter', () => { | |||
extras: { | |||
catalogUuid: 'metawal.wallonie.be', | |||
favoriteCount: 0, | |||
featureTypes: [], |
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.
If i understand well we tested this in the gn4 repository by checking featureType has some elements no ?
) | ||
} | ||
|
||
return of(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.
@jahow comments : keep the "happy path" on the left by checking the null return condition first , then the non null condition after
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.
FYI can't apply comment suggestion as this function has evolved, there is no more test on decodeMap
.
/** | ||
* FeatureCatalog | ||
*/ | ||
loadFeatureCatalog(metadata: CatalogRecord) { |
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.
@jahow cmments : we should not need this on the facade because loading the attributes will not be required by the components; instead, an effect in the state should do it
@@ -17,13 +18,18 @@ export interface MetadataViewState { | |||
allUserFeedbacksLoading: boolean | |||
addUserFeedbackLoading: boolean | |||
chartConfig?: DatavizConfigurationModel | |||
featureCatalog?: DatasetFeatureCatalog | |||
featureCatalogLoading: boolean | |||
featureCatalogError: { notFound?: boolean; otherError?: string } | 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.
@jahow comments : I think it'd make sense to have a separate error for the feature catalog loading; this error can only be a string because in the case of feature catalogs it's OK to not find any
Alita, does the notFound necessary ?
@@ -52,6 +52,11 @@ export class MdViewFacade { | |||
filter((md) => !!md) | |||
) | |||
|
|||
featureCatalog$ = this.store.pipe( | |||
select(MdViewSelectors.getFeatureCatalog), | |||
filter((fc) => !!fc) |
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.
@jahow comments : I would remove the filter here as it can be misleading for a consumer of this stream; e.g. if no feature catalog was found, this stream would never emit a value
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.
done
@@ -33,6 +32,31 @@ export class MdViewEffects { | |||
) | |||
) | |||
|
|||
loadFeatureCatalog$ = createEffect(() => |
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.
@jahow comments :
Here I would add an additional effect that emits a loadFeatureCatalogAttributes action when a loadFullMetadataSuccess action is received; this way the loading of the feature catalog will be done automatically when a full record is available, and then repository.getFeatureCatalog(record) can be called
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.
@jahow finally we keept only the loadFeatureCatalog associated to the full metadata loading action as long as there might be a associated effects to feature loading. Are you ok with this behavior ?
) | ||
) | ||
) | ||
|
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.
@jahow says : need tests
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.
test done
9e34c0d
to
b25a6d9
Compare
Description
This PR introduces feature catalog. It is a first technical step to load a feature catalog in the record before displaying it in a record page.
Attributes from a feature catalog can be retrieve in two different ways:
featureTypes
propertyrelated.fcats
property: in this case we will store the linked record uuid as property (featureCatalogIdentifier
) and will load (getRecord()
) its definition to get its ownfeatureTypes
Thanks @Guillaume-d-o for your work on this PR.
💡 Example of a record with a linked feature catalog (for Case 2)
Architectural changes
New mappers were added to load
featureTypes
and get thefeatureCatalogIdentifier
.Screenshots
No UI
Quality Assurance Checklist
breaking change
labelbackport <release branch>
label