-
Notifications
You must be signed in to change notification settings - Fork 421
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 optional collection home page and descriptive collection content fields #3256
base: main
Are you sure you want to change the base?
Add optional collection home page and descriptive collection content fields #3256
Conversation
0f6b9dc
to
131b983
Compare
@alexklbuckley : This PR needs more information about how someone can test this feature. It doesn't appear to have any instructions on how this "collection home page" can be used, and it's unclear (to me) how to begin testing it or what "editable content" is supposed to be displayed. Could you please update the description of this PR to provide instructions for reviewers on how this is supposed to work? Until those details can be added, I've placed this in the "On Hold" column of our 9.0 board. |
131b983
to
53ca700
Compare
fields - Fix failing tests
fields Sponsored-by: Auckland University of Technology, New Zealand
df8619e
to
9aa117d
Compare
fields - Fixing failing build. Sponsored-by: Auckland University of Technology, New Zealand
39d9931
to
a4db843
Compare
Thanks @tdonohue , I've updated the description of this PR to provide instructions for reviewers on how to test this. I've also noted in the description that the collection home page is optional, and is routed to only if a new collection.routeThrough.collectionHomePage config in the config.*.yml file is set to 'true'. Also, the new collection content fields do enable users to save HTML content. However, that data can only be saved by administrators and is handled in the same way as the collections dc.description does. One thing I am not sure about, is how can I add a new metadata schema and then fields to that schema. Currently, in my test plan (in the PR description) steps 13 and 14 I am asking the tester to add that through DSpace Admin Registries > Metadata. But is there a better way? |
a4db843
to
8836a88
Compare
fields - Trying to fix further failing unit tests Sponsored-by: Auckland University of Technology, New Zealand
8836a88
to
41e788d
Compare
@alexklbuckley : Metadata fields should be added on the backend via a metadata schema registry file in this folder: https://github.com/DSpace/DSpace/tree/main/dspace/config/registries For instance, the entire "dc" list of fields is in https://github.com/DSpace/DSpace/blob/main/dspace/config/registries/dublin-core-types.xml In this case, it appears you are defining a new "collection" metadata schema, which means it'd need its own "collection-types.xml" file (or similar) to define that schema. Then, it'd need to be added to the list of "registry.metadata.load" setting in dspace.cfg: https://github.com/DSpace/DSpace/blob/main/dspace/config/dspace.cfg#L925-L945 (As that's the list of metadata configs which DSpace will load during installation/upgrade) For now, your manual process seems sufficient to have others test out this PR and provide feedback. However, assuming there's agreement in this direction, we'd likely need to have a corresponding backend PR created which adds a new "collection-types.xml" file or similar. You can wait to create that secondary PR though if you wish to simply get early feedback on this PR first. |
thank you for that information @tdonohue ! Yes, I will wait to create that secondary PR to get early feedback on this PR first. I've got all the unit tests passing on this PR now. I am not sure about the remaining checkboxes in the checklist, so any guidance on that would be very much appreciated! |
Since the new metadata fields are specific to DSpace, should they not go into the 'dspace' schema? Or is this meant to be a general-purpose namespace, published for other repository products to adopt? |
Aside: the registry editor in DSpace is handy for building and tweaking a new namespace, and I've been meaning to write a registry "unloader" to complement the registry loader which is run during installation (which loading is why a schema definition file is required). |
@mwoodiupui : Good point, these metadata fields likely should go in the |
fields: Implement review requests - Change to fetching collection content fields from dspace.collection.* metadata fields. Sponsored-by: Auckland University of Technology, New Zealand
- This commit adds the metadata fields to the existing dspace metadata schema required for this DSpace Angular PR#3256: DSpace/dspace-angular#3256 Sponsored-by: Auckland University of Technology, New Zealand
Thanks @mwoodiupui and @tdonohue ! I've pushed a follow-up to this PR to use dspace.collection.* fields. I've also now pushed a new PR to DSpace backend to include the 4 new metadata files into the existing 'dspace' schema registry file: DSpace/DSpace#9794 |
Hi @tdonohue are we able to take off the 'needs discussion' label for this pull request? As I have done a follow-up pull request ( DSpace/DSpace#9794 ) addressing the feedback in #3256 (comment) |
@alexklbuckley just a tip about sanitization as that will definitely come up in a review: |
Hi @kshepherd thank you for your reply. As I am new to this, can I please check, is it this method https://github.com/DSpace/dspace-angular/blob/fca5700012ec63f17b1fa645f8ae39af4f9ca122/src/app/shared/utils/markdown.directive.ts#L56C9-L56C15 that you would recommend we could use? If yes, could you please point us to an example of how it is used directly? Thanks so much |
The line I was referring to is at
The first thing I would try is just importing and re-using the markdown directive as it is, directly - even though you're passing HTML in, my understanding is this should be considered valid markdown (I could be wrong), e.g. If that doesn't work, in your backing component, you could inject the DOMSanitizer in your constructor, and implement something similar to the dsMarkdown directive, or just a function that calls the sanitize method, like Where the function is like
(this has the benefit of not trying to render markdown to HTML first). None of this is tested, just concepts/rough templates of things to try. |
Quick update to this - I just worked on a feature where I was including some copyright/attribution HTML in an element, and the I just opened a quick PR to fix this, so you might find it useful for your local code, too, both as a fix for the markdown directive if you plan to use it, or a direct example of using DOM sanitizer: #3375 |
10b7337
to
d662acc
Compare
Thanks @kshepherd I wrote a followup which implemented the first of your above suggestions but that didn't look to be working. I've now pushed a follow-up implementing the second of your suggestions. Would you mind please taking a look if how I am doing it now is correctly sanitising the inputted content? |
References
Add references/links to any related issues or PRs. These may include:
Description
Add new optional collection home page to display new descriptive collection content inputs.
This collection home page is routed to from the 'Subcommunities and collection' page of a community IF the DSpace Angular config.*.yml file has the following config collection.routeThrough.collectionHomePage = true.
All new collection content fields are only editable to Administrators on the DSpace Admin Collection 'Edit Metadata' form.
Instructions for Reviewers
List of changes in this PR:
Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.
In your web browser open the DSpace Angular home page
Go to: Communities & Collections
Click on a community
Click on the 'Subcommunities and collections' tab of the Browse element.
A 'Collections in this community' page will load. Hover over the collection links and notice they are formed as: URL/collections/. For example: http://localhost:4000/collections/282164f5-d325-4740-8dd1-fa4d6d3e7200
Edit your config/config.*.yml file. Find '# Collection Page Config'. Underneath that set:
So your collection config looks like:
Restart and recompile your dev environment for this config change to take effect
Repeat steps 1, 2, 3, 4
The 'Collections in this community' page will load. Hover over the collection links and notice they now have /home on the end. For example: http://localhost:4000/collections/282164f5-d325-4740-8dd1-fa4d6d3e7200/home
Click on the collection link and notice a page successfully loads. From there you can browse the collection. You can add more custom content fields to this collection home page, which we will do next.
11A. If you have a local DSpace backend install, then checkout DSpace/DSpace#9794 to install 4 new metadata fields.
11B. If you do not have a DSpace backend install then add new metadata fields manually:
<h1>Welcome to the Mathematics collection!</h1>
This collection is managed and curated by the Department of Mathematics
Mary Smith
Then navigate back to 'Subcommunities and collections' tab of the parent community.
Click on your collection link again. The /home page will load and the content you added into the 4 input fields in step 13 is now displaying. HTML content you entered is being rendered correctly.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
main
branch of code (unless it is a backport or is fixing an issue specific to an older branch).yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.