-
Notifications
You must be signed in to change notification settings - Fork 0
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
Provide backbone-defined valid tags for Attributes #307
Conversation
You can already review. The backbone route is not yet finished, so this PR will still be a draft |
Getting the list of valid tags doesn't really require being logged in, on the other hand they're only supposed to be used for attributes which currently can only be shared between Identities. So I think it's ok that they require a login. But I'd expect that the list is not fetched from the backbone every time you want to do something with a tag (which is often), but rather that the backbone sends the list whenever it is changed (e. g. via an event) and it's then stored locally. |
on a second thought: the tag list is only required when creating or receiving an attribute with tags or when changing tags. I'm not sure where this falls regarding storage vs. no storage |
I'll not review a Draft. Please re-assign when finished. |
https://github.com/nmshd/backbone/releases/tag/6.16.0 supports tags |
packages/runtime/src/extensibility/facades/transport/TagsFacade.ts
Outdated
Show resolved
Hide resolved
876cf62
to
d8f5f70
Compare
packages/consumption/src/modules/attributes/AttributesController.ts
Outdated
Show resolved
Hide resolved
packages/consumption/src/modules/attributes/local/AttributeTagCollection.ts
Outdated
Show resolved
Hide resolved
packages/runtime/src/useCases/consumption/attributes/GetAttributeTagCollection.ts
Outdated
Show resolved
Hide resolved
packages/consumption/test/modules/attributes/AttributeTagCollection.test.ts
Outdated
Show resolved
Hide resolved
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.
LGTM
@jkoenig134 Do you want to take another look?
@sebbi08 now the things in transport are also named If that's undone I will do another review. |
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.
LGTM
Readiness checklist