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

Fix collections view issue #127

Closed
wants to merge 1 commit into from

Conversation

atarix83
Copy link
Contributor

@atarix83 atarix83 commented Jul 25, 2017

This connect to #122

This resolve issue with the collections listeed in the community page and added also pagination.

Rename the HAL link
@ghost ghost assigned atarix83 Jul 25, 2017
@ghost ghost added the needs review label Jul 25, 2017
@@ -16,6 +16,7 @@ import { hasValue } from '../shared/empty.util';
})
export class CommunityPageComponent implements OnInit, OnDestroy {
communityData: RemoteData<Community>;
connumityId: string;
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be communityId

@@ -90,4 +101,15 @@ export abstract class DataService<TNormalized extends CacheableObject, TDomain>
// return this.rdbService.buildSingle(href));
}

findEmbedded<TEmbeddedNormalized extends CacheableObject, TEmbeddedDomain>(
Copy link
Member

Choose a reason for hiding this comment

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

It isn't necessary to write a method to specifically fetch embedded resources in a response. If, for example, you want the collections that were embedded in a community response, all you need to do is first fetch the community, and later do a collectionDataService.findAll scoped to that community. If the collections were embedded in the first response, they will be cached, and no new request will be made.

Copy link
Contributor Author

@atarix83 atarix83 Jul 26, 2017

Choose a reason for hiding this comment

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

Yes you're right, but there isn't a browse endpoint for collections so this rest call doesn't work:

http://dspace7.4science.it/dspace-spring-rest/api/discover/browses/dateissued/collections?scope=7669c72a-3f2a-451f-a3b9-9210e7a4c02f

The right way to retrieve collections inside a community is this call :

http://dspace7.4science.it/dspace-spring-rest/api/core/communities/7669c72a-3f2a-451f-a3b9-9210e7a4c02f/collections

That's why I did the new method

Copy link
Member

@artlowel artlowel Jul 26, 2017

Choose a reason for hiding this comment

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

There is a /collections endpoint, so you should be able to do:
http://dspace7.4science.it/dspace-spring-rest/api/core/collections?scope=7669c72a-3f2a-451f-a3b9-9210e7a4c02f if the scope param isn't implemented on that endpoint on the rest side, we shouldn't work around it in the UI, but wait for it to be implemented.

I don't believe UI components should know about the way a resource is sent by the rest API, whether it's embedded or not, whether it needs to be fetched from a different URL. It should only know about the data services. If it needs a collection, it should address the collectionDataService and ask for it, everything else should be abstracted away, otherwise the code becomes too coupled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case we should change browse endpoint for collections now is /discover/browses/dateissued/collections

So I will change the code in the way that you have suggested and wait for the rest API implementation.

We should report this to @abollini and the rest API team

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't understand why the /discover/browses/dateissued endpoint come in this discussion :(
I can say that the right endpoints to retrieve all the collections (paginated) within a community is like
http://dspace7.4science.it/dspace-spring-rest/api/core/communities/7669c72a-3f2a-451f-a3b9-9210e7a4c02f/collections
another endpoint could be added if needed (I don't see a reason for that honestly) but should be like
/collections/search/findByCommunity?scope=:uuid

Copy link
Member

Choose a reason for hiding this comment

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

I have added some additional information about the "search" rels in the rest contract https://github.com/DSpace/Rest7Contract/blob/master/search-rels.md
this idea comes from the Spring Data Rest project, see https://docs.spring.io/spring-data/rest/docs/current/reference/html/#repository-resources.search-resource

Copy link
Member

Choose a reason for hiding this comment

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

sorry for the late comment. I have reviewed the issue with @atarix83 and unfortunately it is much more complex than what we initially expect.
We can for sure implement the /collections/search/findByCommunity?scope=:uuid and this would probably make possible to fastly solve the issue but inspecting the code I have understood that our angular ui is currently build without taking advantage of the HAL support that we are forcing in the rest part.
In a HAL response you receive a list of links to follow, and the client should be written without previous knowledge about the target URL. In the best scenario the client doesn't need to know anything but of course this lead to very generic client such as the HAL browser. The normal scenario requires that the client (angular-ui) only know about the name of the links to follow, so in this case collections and it should discover the URL of the endpoint looking to the response.
The current services in the angular-ui project have constants were the endpoints are hardcoded, and most danger also "sub-endpoints" are constructed on the angular side.
Also the "main" endpoints can be discovered at runtime just looking to the /api ROOT HAL document.
Making use of the HAL links we will make the angular-ui resilient to endpoints reorganisation but more important we will allow extension or customization on the REST part using different strategies. An institution could provide an additional enhanced endpoint to retrieve the collections without removing the out-of-box implementation but only forcing the HAL document to point to the new endpoints. The angular-ui or some components could be build to inspect the HAL response for not-yet known links and react to them calling external plugins...

I suggest to start implementing a followLink method in the data.service.ts that will accept a
HALdocument and a linkName and will return a HALdocument.

@ghost
Copy link

ghost commented Sep 29, 2017

It appears there are pending change requests on this pull request. Any update @atarix83?

@atarix83
Copy link
Contributor Author

atarix83 commented Oct 3, 2017

@wwelling
this issue needs to be review now that this issue has been merged

@ghost
Copy link

ghost commented Oct 3, 2017

@atarix83 would you be willing to refactor the misspelling of community and resolve the merge conflicts? After which I will gladly review and test.

@@ -22,5 +22,5 @@
[content]="(communityData.payload | async)?.copyrightText"
[hasInnerHtml]="true">
</ds-comcol-page-content>
<ds-community-page-sub-collection-list></ds-community-page-sub-collection-list>
<ds-community-page-sub-collection-list [communityId]="connumityId"></ds-community-page-sub-collection-list>
Copy link
Member

Choose a reason for hiding this comment

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

Misspelling of communityId here too

@atarix83
Copy link
Contributor Author

atarix83 commented Oct 3, 2017

Now code is too old and needs to be refactored after new updates, I will inform you as soon as I have done

@artlowel
Copy link
Member

artlowel commented Oct 3, 2017

Please wait until the follow-up PR to #147 , that will do the same for browse endpoints is ready.

@tdonohue
Copy link
Member

tdonohue commented Mar 8, 2018

Closing this PR as obsolete / outdated. I'll move the corresponding ticket back into "ready" state/column.

@tdonohue tdonohue closed this Mar 8, 2018
@ghost ghost removed the needs review label Mar 8, 2018
@atarix83 atarix83 deleted the collection-issue-new branch March 9, 2018 13:02
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Mar 11, 2022
[DSC-469] Add dc.type field to administrator bitstream edit page

Approved-by: Giuseppe Digilio
kosarko pushed a commit to ufal/dspace-angular that referenced this pull request May 11, 2023
Co-authored-by: MilanMajchrák <milan.majchak@dataquest.sk>
kosarko pushed a commit to ufal/dspace-angular that referenced this pull request Jun 20, 2024
Co-authored-by: MilanMajchrák <milan.majchak@dataquest.sk>
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.

4 participants