Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: adapt policy model to ODRL #87

Merged
merged 1 commit into from
Sep 7, 2023
Merged

feat: adapt policy model to ODRL #87

merged 1 commit into from
Sep 7, 2023

Conversation

ndr-brt
Copy link
Contributor

@ndr-brt ndr-brt commented Sep 4, 2023 •
edited
Loading

Description

Adapt policy model to ODRL

notes

  • moved policy related files to a dedicated folder.
  • removed "todo" tests because we don't need to test the EDC internal logic, only the communication layer.

Closes #86

fdionisi
fdionisi reviewed Sep 4, 2023
View reviewed changes
src/entities/policy/policy.ts
'@id'?: string;
id?: string;
policy: Policy;
get permissions(): JsonLdObject[] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we keep naming alignment (i.e., permission v permissions)?

Copy link
Contributor Author

@ndr-brt ndr-brt Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, ODRL asks for the singular, but tbh a method that returns a list in my opinion should be plural for readability.
Currently we have this "interface model" for building request objects that must follow the protocol 1:1, instead, on the response side we have this json-ld wrappers that permits to define a more dev-friendly interface.
My take on this is that we could (should?) switch to the second approach for the input as well, but I know that js developers love that interface-objects stuff :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel it's the input as the interface that is helpful for avoiding the boilerplate of creating a new instance every time and directly using an object. But I understand your thoughts. Thank you for the explanation

@ndr-brt ndr-brt requested a review from fdionisi September 4, 2023 09:34
fdionisi
fdionisi approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@fdionisi fdionisi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is looking good - possible to coordinate with #89 though, but I trust you have it under control. 💪

@ndr-brt ndr-brt merged commit a8d4084 into main Sep 7, 2023
@ndr-brt ndr-brt deleted the 86-jsonld-policy-definition branch September 7, 2023 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fdionisi fdionisi fdionisi approved these changes

@OlfaBensoussia OlfaBensoussia Awaiting requested review from OlfaBensoussia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adapt Policy model to ODRL
2 participants
@ndr-brt @fdionisi