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.

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

Add missing ResourcePolicyService #427

Merged
merged 5 commits into from Jul 18, 2019

Conversation

AlexanderS
Copy link
Contributor

This adds the missing ResourcePolicyService. The call to this.collectionDataService.findByHref only succeeds, because the full href is used and typescript does not check the return type.

I do not know if the resouce-policy.service.ts file should be placed in something like src/app/core/authz/, but it's only a single file.

Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @AlexanderS !

Would you mind writing a spec file for resource-policy.service.ts? It doesn't have to be very complicated, just make sure that every new method is covered.

@atarix83 Since this is a change to the submission, and you know a lot more about its inner workings than I do, perhaps you could give it a review?

src/app/core/data/resource-policy.service.ts Outdated Show resolved Hide resolved
@atarix83 atarix83 self-assigned this Jul 11, 2019
Copy link
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

Thanks @AlexanderS for this PR and thanks @artlowel to mention me

@AlexanderS
you have created a DataServiceImpl both for MetadataSchemaDataService and ResourcePolicyService. Is this delegation strictly necessary? If so I should separate classes in separated file as raccomended in Angular style guide https://angular.io/guide/styleguide#single-responsibility.

other than this there is a TypeScript error in resource-policy.service.spec.ts file to resolve

@AlexanderS
Copy link
Contributor Author

@atarix83 I just fixed the typescript error.

The DataServiceImpl stuff was just inspired by the DSpaceObjectDataService (see this conversation with artlowel) I can revert this change and remove the delegation and directly extending DataService again, if you like.

@atarix83
Copy link
Contributor

@AlexanderS thanks for solving the typescript error.

I hadn't seen the conversation with @artlowel, since we already have similar implementations please leave the code as it is although we do not follow Angular style guide

Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @AlexanderS!

LGTM

@atarix83 you're right, these DataServiceImpl classes technically violate that rule in the style guide. But I don't think they violate the spirit of the rule: that each file should do one thing. All @AlexanderS did was split the code that was in that file anyway into a private and a public part.

While we could move that DataServiceImpl class to a separate file, it should never be used by anything other than ResourcePolicyService, so there isn't much of a point in my opinion.

@artlowel artlowel merged commit 5be883f into DSpace:master Jul 18, 2019
@tdonohue tdonohue added this to the 7.0beta1 milestone Jan 26, 2021
kosarko pushed a commit to kosarko/dspace-angular that referenced this pull request Apr 3, 2024
* Do not show licenses if the Item doesn't have any file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants