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

Edit homepage news #45

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

benbosman
Copy link
Member

This PR provides a home page introduction text which can be modified and retrieved over REST.
This is designed to access a list of all static pages (currently home page news only). These are pages not specific to any items. The design of the REST contract should allow it to be extended to e.g. images as well, or multiple home-page sections similar to the JSPUI.
Because the XMLUI supports home page news per language, multiple pages can be created here as well.

## Main Endpoint
**/api/config/pages**

Provide access to the list of all static pages (currently home page news only). These are pages not specific to any items. The design of the REST contract should allow it to be extended to e.g. images as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know Lieven said it wasn't with high priority. But reading this text, it seems to be in an earlier stage.

Copy link
Member Author

Choose a reason for hiding this comment

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

The contract is actually not in an early phase.
The contract was designed to support the various pages without changes to the contract, but the Java implementation will be limited to the home page news for DSpace 7. This may be extended in DSpace 8, and the goal is to achieve this without changes to the contract

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thank you for the clarification!

Copy link
Member

Choose a reason for hiding this comment

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

we already have a partial support for additional pages, see https://github.com/DSpace/DSpace/blob/dspace-6_x/dspace/config/spring/api/core-services.xml#L60
so we should provide a basic solution that still allow institutions to easily add new page with minimal effort.

Trying to design for the future without enough information about what and how we will go to implement is risky and can easily turn in over-engineering the current needs and provide something that we will found sub-optimal when really needed. If this will be the case we will be constrained by this premature design or we will end to broke the contract anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

This contract has been designed specifically to ensure institutions can easily add a page with minimal effort.
This is exactly the reason for the approach where static pages can be created/downloaded.
So contrary to the old JSPUI config you pointed at, the ability to create static pages won't require any changes in the backend.
All that's required is a POST of a new page with a different name.

homepage-news.md Outdated
"format": {
"href": "https://dspace7.4science.it/dspace-spring-rest/api/config/pages/004a297e-fd06-4662-ae51-73e4b7c165c8/format"
},
"format": {
Copy link
Contributor

Choose a reason for hiding this comment

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

how to differentiate the 2 formats, or instead this is a language?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, this was a typing error. I've updated it now since this is a link to the languages

Copy link
Contributor

@paulo-graca paulo-graca left a comment

Choose a reason for hiding this comment

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

Thank you Ben!

Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

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

Thanks to start the conversation about the support for the "Edit homepage news" content.
As you can read also in the inline comments I really worried about attempt to design for the future when there are still a large level of uncertainty that can easily lead us to fail in set a contract that will be good for our future needs.

Moreover, the current proposal also requires some additional development as we don't have the concept of uuid for the "news content" nor a title.
While I feel valuable explore the possibility to use bitstreams***** to store this content in future, especially if we are going to support images, css, javascript and other media I'm not sure that it is a goal that we want / we are able to set for DSpace 7.
*Please note that the proposal doesn't say that explicitly it is my interpretation (or my own suggestion) as we see the need to a link with the bitstreamFormat

Alternative proposal
As news content is managed as metadata at the community and collection level, why we cannot do the same at the site level? we already have the basic structure in place as we have a SiteObject (with its ridiculous 1 column table ;) ) that extends DSpaceObject so it can have metadata. This will solve also the issue to manage multiple languages in an effective way. Anyway, we can also keep the current filesystem storage and only expose the content of these files as metadata.

When we will be ready to manage images and other content this look to me similar to "logo" in community and collection, so why we cannot do the same for the site as well?

/api/core/sites/:uuid/metadata will return the text of the top news in all the available languages, and we already now how to update it using the patch metadata....
{ "site.home.topnews": [ { value: "This is the content of the top news...", language: "en" }, { value: "Questo è il contenuto della notizia di rilievo...", language: "it" } ] }

I will be happy to formalize the above alternative approach as a PR if you feel it valuable.

[Back to the list of all defined endpoints](endpoints.md)

## Main Endpoint
**/api/config/pages**
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the config category, what about web or cms? (btw maybe it is completely unnecessary to add a category see below)

Copy link
Member Author

Choose a reason for hiding this comment

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

cms is good for me as well

## Main Endpoint
**/api/config/pages**

Provide access to the list of all static pages (currently home page news only). These are pages not specific to any items. The design of the REST contract should allow it to be extended to e.g. images as well.
Copy link
Member

Choose a reason for hiding this comment

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

we already have a partial support for additional pages, see https://github.com/DSpace/DSpace/blob/dspace-6_x/dspace/config/spring/api/core-services.xml#L60
so we should provide a basic solution that still allow institutions to easily add new page with minimal effort.

Trying to design for the future without enough information about what and how we will go to implement is risky and can easily turn in over-engineering the current needs and provide something that we will found sub-optimal when really needed. If this will be the case we will be constrained by this premature design or we will end to broke the contract anyway.

"_embedded": {
"pages": [
{
"uuid": "004a297e-fd06-4662-ae51-73e4b7c165c8",
Copy link
Member

Choose a reason for hiding this comment

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

how will we get that? where we will store the mapping between the uuid and the name that is what currently identify the page?

Copy link
Member Author

Choose a reason for hiding this comment

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

The storage of the static pages will happen in the database (uuid, name, title, language, bitstream)

"uuid": "004a297e-fd06-4662-ae51-73e4b7c165c8",
"name": "home-page-news",
"title": "DSpace Demo Repository",
"sizeBytes": 234,
Copy link
Member

Choose a reason for hiding this comment

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

this is not really the size of the page but just the size of the fragment. Also in future when we will be able to deliver user defined content as an additional page we will probably include his content inside the default theme of DSpace making impossible to estimate the page size in advance and mostly useless the information about the size of the fragment... it could become useful when the "page" is a css, js or other media

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed the size of the fragment, and the user will download that fragment.
Angular will integrate the fragment in the theme

For HTML the fragment is indeed often small, but storing the size does ensure it's immediately useful for other media, and no additional effort

homepage-news.md Outdated
"_embedded": {
"languages": [
{
"lang": "en"
Copy link
Member

Choose a reason for hiding this comment

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

we need to find a way to reference the alternative languages so that the UI will be able to load the appropriate content if the user switch the locale

Copy link
Member Author

Choose a reason for hiding this comment

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

that's correct, I have forgotten the links here. This has been solved now

}
},
"_embedded": {
"format": {
Copy link
Member

Choose a reason for hiding this comment

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

this seems to suggest the use of bitstream in future to store our pages/page fragments/media.
I like this idea but again if we are not going to do that now, I suggest to just model what we have now.

@benbosman
Copy link
Member Author

@abollini I think I can answer briefly on this to solve your concerns:

  • Yes, the plan is to work with bitstreams
  • And yes, the functionality will already immediately support storing more than just the home page news, but it's the UI section which will not be implemented for anything beyond the home page news.

I hope this clarifies the contract

@tdonohue tdonohue self-requested a review February 12, 2019 20:41
@tdonohue
Copy link
Member

@benbosman : I'd like to better understand the implementation suggestions/vision here before I can comment on this contract. While this new feature is important, there's a real risk of "scope creep" (or overengineering, as @abollini noted) if we are planning to overhaul how this already works.

As you are already aware, I'm trying to limit the amount of significant changes to the underlying DSpace (Java) API in DSpace 7 (as the real goal is to concentrate on the REST API and Angular UI). Therefore, I'd like to see a more detailed proposal / brainstorm (even just a wiki page of notes or Google doc) for how we'd implement this feature, so that I can provide better feedback.

@abollini
Copy link
Member

Thanks @benbosman to confirm my understanding about your plan to use bitstreams here. Please note that there are many other comments and point of discussions in my review above where I will like to know your opinion. Specifically, I would like to receive clarification about how you plan to assign an uuid to the current news and why if they will be bitstreams we need a separate endpoint for them instead than just use / link the bitstream endpoint itself. Moreover, I will like to know what you think about the alternative approach that should be IMHO more easy to implement and more aligned with what we currently have and do for communities and collections.

Copy link
Member Author

@benbosman benbosman left a comment

Choose a reason for hiding this comment

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

I've included some inline comments to answer the detailed questions.

The overall plan here is to:

  • Implement this in the backend to be flexible. This is very easy to achieve since we already have bitstreams which can contain any content
  • Make sure the REST Contract supports multiple languages, multiple pages, and logos, styling, …. By using bitstreams, we can handle all of this and immediately identify the applicable content type
  • Keep the Angular implementation limited to the home page HTML news. This is where the complexity of supporting other features would occur. By keeping this limited, the scope remains small. But by ensuring the backend can handle more, we also ensure that in DSpace 8 we can support more in Angular without having to start from scratch again for both the backend and frontend

I hope this helps understanding why this is a simple implementation (much simpler than most of the PRs we already contributed), but is designed to be flexible.

## Main Endpoint
**/api/config/pages**

Provide access to the list of all static pages (currently home page news only). These are pages not specific to any items. The design of the REST contract should allow it to be extended to e.g. images as well.
Copy link
Member Author

Choose a reason for hiding this comment

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

This contract has been designed specifically to ensure institutions can easily add a page with minimal effort.
This is exactly the reason for the approach where static pages can be created/downloaded.
So contrary to the old JSPUI config you pointed at, the ability to create static pages won't require any changes in the backend.
All that's required is a POST of a new page with a different name.

"_embedded": {
"pages": [
{
"uuid": "004a297e-fd06-4662-ae51-73e4b7c165c8",
Copy link
Member Author

Choose a reason for hiding this comment

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

The storage of the static pages will happen in the database (uuid, name, title, language, bitstream)

"uuid": "004a297e-fd06-4662-ae51-73e4b7c165c8",
"name": "home-page-news",
"title": "DSpace Demo Repository",
"sizeBytes": 234,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed the size of the fragment, and the user will download that fragment.
Angular will integrate the fragment in the theme

For HTML the fragment is indeed often small, but storing the size does ensure it's immediately useful for other media, and no additional effort

homepage-news.md Outdated
"_embedded": {
"languages": [
{
"lang": "en"
Copy link
Member Author

Choose a reason for hiding this comment

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

that's correct, I have forgotten the links here. This has been solved now

[Back to the list of all defined endpoints](endpoints.md)

## Main Endpoint
**/api/config/pages**
Copy link
Member Author

Choose a reason for hiding this comment

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

cms is good for me as well

@tdonohue
Copy link
Member

Hi @benbosman : I would still like to better understand the higher level design/proposal here before I can comment further. I have too many questions about what you are proposing...here's just a few:

  • Where would a Bitstream containing Homepage news be kept? Are you implying there's now a special type of Item (or Collection) that stores this information? Is a Bitstream now attached to a Site Object? If there can be more than one of these Bitstreams, how do you identify which one is the Homepage News file vs something else (Do individual Bitstreams need special names/titles)?
  • What does the workflow for updating this information look like (for an Administrator)? How would Homepage News get updated? For example, is there an Admin UI, or are they expected to upload a (manually updated) Bitstream?
  • Are we validating the content of these Bitstreams in any way? For example, presumably homepage news needs to be in HTML format, and not a Word document or PDF.

Generally speaking, I don't understand what the proposed process is for creating and updating these special bitstreams, nor where they are stored within DSpace's object model. So, I'd like to see the high level proposal defined in a Wiki page or Google Doc and linked into this PR.

@abollini
Copy link
Member

abollini commented Mar 7, 2019

Hi @benbosman thanks to have added some details. From your latest comments I understand that you plan to create a new Object "Page" that will be linked to a bitstream that will actually hold the content.

I don't see a reason to do that, it could be eventually done just using the bitstreams without the need to introduce a new object. The site object can expose links to such bitstreams similary to how collection and community deal with logo.

Again, I strongly suggest to don't introduce new things until we know why these new stuff is needed and useful. If we diverge from a current implementation (i.e. how the news and logo are managed at the collection and community level) I would like to see a clear path to end with a better uniform solution. If we are not satisfied from what we actually have we should clearly state that and set a goal for its improvement otherwise we will end with two different approaches and the next time a third could raise.

The natural implementation for this feature is imho based on the use of metadata and bitstreams as we have done for community and collections. Again, in this way we will solve the problem of multilanguages for the news (metadata) and we will deal with the multilingual issue of the "media" (images, videos, etc.) using some sort of links between bitstreams that is what we also need for (thumbnails-original files).

@tdonohue tdonohue removed their request for review June 29, 2020 15:54
@heathergreerklein heathergreerklein added this to Needs Reviewers Assigned in DSpace 7.1 release via automation Jul 10, 2020
@heathergreerklein heathergreerklein added this to the 7.1 milestone Jul 10, 2020
@heathergreerklein heathergreerklein moved this from Needs Reviewers Assigned to Under Review in DSpace 7.1 release Jul 10, 2020
@tdonohue tdonohue changed the base branch from master to main July 13, 2020 20:45
@tdonohue tdonohue removed this from the 7.1 milestone Jul 15, 2021
@tdonohue tdonohue removed this from Under Review in DSpace 7.1 release Jul 15, 2021
@tdonohue tdonohue added this to Needs Reviewers Assigned in DSpace 7.x Tier 5: Low Priority via automation Jul 15, 2021
abollini added a commit to 4Science/Rest7Contract that referenced this pull request Nov 10, 2023
CST-12467 refactor qatopic to always refer to a qasource
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
DSpace 7.x Tier 5: Low Priority
  
Needs Reviewers Assigned
Development

Successfully merging this pull request may close these issues.

None yet

5 participants