[DSpace-CRIS] Port CMS edit and User Agreement edit from DSpace CRIS#5106
[DSpace-CRIS] Port CMS edit and User Agreement edit from DSpace CRIS#5106Dawnkai wants to merge 7 commits intoDSpace:mainfrom
Conversation
|
When creating this PR, I decided to limit the amount of edits that I made to the code in CRIS and tried to port it as close to 1:1 as possible. This was caused by the time restriction (and busy work week). I have a few potential ideas on how to improve this solution in mind, and I will add them in the code in the form of comments for further potential discussion. |
|
@Dawnkai : Thanks for creating this PR! Your approach sounds correct. I'd recommend keeping this PR as close to 1:1 port of code as possible. That said, you are welcome to add suggestions for changes here for discussion. Or, you also could create a new ticket to describe the improvements (linking them back to this PR). Assuming we all can agree on the improvements, those could always be made in a future, follow-up PR. (As a sidenote, it looks like some automated tests are failing in this PR. However, the error looks to me like it could be a random failure, so I've triggered a re-run of the tests) |
|
@Dawnkai : After rerunning the tests, they are now returning a different error. It looks like this new error is saying that a few unit tests / specs are broken. Could you look into that and get them fixed? |
Fix errors in testing introduced by PR: DSpace#5106 by replacing Store mocking with mocking of the SiteService.
|
@tdonohue I fixed it. The problem was that in the old version in the tests for the footer component an empty store was mocked, which was a problem - because in CRIS code the component is making a request (and thus fetching non-existing request cache from it). I mocked the SiteService (which is where the request was made) instead. Now it works. Now all the tests pass. Considering you want 1:1 port (which is of course not fully possible), here is a list of changes I made compared to a "pure" CRIS port:
So in terms of functionality, no changes were introduced. |
|
Thanks @Dawnkai ! Just to clarify, I realize a 1:1 port will never be fully possible when merging these DSpace-CRIS features back to DSpace. The Merger Management Team has just made a decision that none of these ports can change the feature's behavior in any significant way. But, we do realize that some code may require updates to be compatible with DSpace... and those small changes for compatibility purposes are welcome. Thank you for describing the changes you had to make. At a glance, those all look like changes for "compatibility purposes" and would be allowed. |
|
@Dawnkai Thanks for the PR. I tested it locally and it worked as inspected. The only issue I had was the display of the cris.cms.home-header:
As someone who don't really know any DSpace-CRIS-System, I wondered, why the position of the header is outside of our normal container-borders, but as this is the described behavior, I'll agree withe PR in general 👍🏾 |
vins01-4science
left a comment
There was a problem hiding this comment.
Hello @Dawnkai, thank you for supporting us with this PR.
The development works as expected, I've only found only few problems:
- The user agreement says that can be fullfilled in MD style, but in reality it can be fullfilled only with HTML syntax:
I think it's okay, but we may need to explicitly say that.
- The Save button on end-user-agreement page ( after the login ) is not working anymore. If I click it won't load anything at all, it just shows a loading spinner endlessly ( the patch is sent to the BE, so if I manually change the route it loads the page I requested correctly ). redirect-url
- I've noticed that also the CMS metadata rendering are not able to display the MD, that it's okay, but I think it would be better to specify that as of now only HTML would be rendered correctly.
- I think we need to change metadata definition, at least for these ones that are new metadata and easily replaceable by using the
dspaceschema.
|
Update from me about the progress and response to the comments so far: @EikLoe Yes, it also irks me, but just as instructed - I tried to limit the altering of CRIS code to the absolute minimum. CRIS has a different format of displaying that section of the page because 4Science is using dynamic components to specify what is to be displayed and in regular DSpace it's hard coded into the HTML. I can apply the container styles to that section of the home page, but only if I am allowed to make such change by @tdonohue. @vins01-4science Thank you for your feedback. To address your points:
Are you sure? Remember that you need to set the environment variable called
Yes, that is true. I am trying to fix it now. It seems to be related to caching and subscriptions in the
I edited the metadata used for CMS on both frontend and backend PR to use |
|
@Dawnkai : I've not had time to look at this PR myself, so I'm unclear on the exact details of the display issues that you and @EikLoe are referencing (and the screenshot @EikLoe previously shared isn't loading for me). However, to clarify, you are welcome to make any changes that help this feature to work better within DSpace. So, if style changes are necessary, please make them. The only changes that the Merger Management Team will not accept are changing the feature behavior... the feature's behavior must remain "as-is". In other words, we want to avoid redesign of the feature itself during migration from DSpace-CRIS to DSpace. Hopefully that clarifies things. But, if I'm misunderstanding the question, then please let me know. |
@tdonohue Oh, sorry. I updated my comment. It should be displayed correctly now. |
|
@Dawnkai: Based on @EikLoe 's updated screenshot, I think that display is also incorrect. Obviously, the header shouldn't appear in a sidebar, so that's something that should be fixed within this PR. So, please do update this PR to get the header to display in the proper location for DSpace. |
|
Hi @Dawnkai, |
|
@Dawnkai : Could we get this PR and the backend PR updated / cleaned up (both currently have merge conflicts)? Our 10.0 feature deadline is Friday, and I'd like to see if we can move this forward. Thanks! |
Fix errors in testing introduced by PR: DSpace#5106 by replacing Store mocking with mocking of the SiteService.
315b7c2 to
e4c5612
Compare
|
@tdonohue I am updating both frontend and backend now. As for the issue mentioned by Vincenzo, I don't have good news. const response$: Observable<RemoteData<T>> = this.rdbService.buildSingle<T>(requestHref$, ...linksToFollow).pipe(
// This skip ensures that if a stale object is present in the cache when you do a
// call it isn't immediately returned, but we wait until the remote data for the new request
// is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
// cached completed object
skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)),
this.reRequestStaleRemoteData(reRequestOnStale, () =>
this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
);The request for new user object never finishes. Fixing this without affecting other components is problematic. I asked another coworker of mine who is much better at angular than me for help, but I am not sure if we will be able to get it fixed by Friday. The only "fix" that works is replacing this fragment in the if (isNotEmpty(redirectUrl)) {
this.router.navigateByUrl(decodeURIComponent(redirectUrl));
}which is just redirecting the user to the proper page after they accepted the agreement, into a hard redirect, which will still do the redirect, but also refresh the page: if (isNotEmpty(redirectUrl)) {
const fullRedirectUrl = `${this.hardRedirectService.getCurrentOrigin()}${decodeURIComponent(redirectUrl)}`;
this.hardRedirectService.redirect(fullRedirectUrl);
}This will force the frontend to reload the user object and everything works well from that point onwards. However, since that is a change (albeit small) to the DSpace functionality, I need confirmation if that is okay. If not, then we will just keep trying to find the source of the issue with my coworker and try to implement a proper fix. |
|
Hi @Dawnkai, |
Fix errors in testing introduced by PR: DSpace#5106 by replacing Store mocking with mocking of the SiteService.
e4c5612 to
3de671e
Compare
3de671e to
27dd2ed
Compare
|
Okay, so: He also found the culprit and has come up with a fix. The issue is caused by recursive calls to /eperson endpoint that are cancelled all at once in RequestService but still inserted into the cache, which results in that permanent ResponsePending status. He also found out that this isn't the first time this issue happens, as he found other attempts at fixing this in git history. In any case, we decided to go forward with the fix I proposed: using hard redirect instead of regular My coworker has a better fix for this problem, however it can potentially contaminate other components, so he will simply make a new bug Issue where he will describe the problem in more detail and provide a fix. Then hopefully we can get it tested thoroughly since we won't be pressured by the DSpace 10 Release Deadline. I tried moving around the site after accepting the user agreement with the hard redirect and found no issues, but if someone wants to give it another test that would be nice. If not then that is all we can do for now. Thanks to everyone involved for the feedback and helpful tips again. |
27dd2ed to
8350f50
Compare
|
Thanks for the update @Dawnkai ! That all sounds good to me. I'm also glad to hear your colleague has found a possible fix & look forward to seeing that bug fix PR once it's ready. I'll try to test this PR later today myself, so that we can try to get this feature into 10.0. Even if it isn't possible to merge today, the Merger Management Team already approved an extension for merger-related PRs until next Wednesday. So, if you can help to continue to address any feedback until then, I'll also do my best to ensure this keeps moving forward so that we can get it into 10.0. Thanks again. |



References
Description
Port CMS metadata editing and user agreement editing as part of CRIS merger.
Instructions for Reviewers
This PR ports two functionalities: CMS metadata editing, and User Agreement editing from DSpace CRIS. I prioritized moving code from CRIS to DSpace as is, without drastic changes in logic and functionality. However, I can see a few places for improvement.
List of changes in this PR:
AdminEditCmsMetadatahas been added. This view is responsible for editing metadata on the Site object, which will then be displayed in selected parts of DSpace.AdminEditUserAgreementhas been added. This view is responsible for editing the user agreement metadata, which is displayed instead of the usual User Agreement text.FooterComponent,HomeNewsComponent,HomePageComponentandEndUserAgreementContentComponent.metadata-deletionprocess on the backend. This means that all users will be forced to accept UA again.In order to test this PR, you have to first import the metadata registry entries from DSpace/DSpace#11896.
CMS metadata edit
Then, when you log in as an admin, you should see two new buttons on the admin panel:
Then, you should be able to select three metadata values:
cris.cms.home-header,cris.cms.home-newsandcris.cms.footer. Select the value you wish to edit. I recommend selectingcris.cms.home-header, as that one will be visible right away. Then, type something in the textarea for the langauge of your choice:And then click

Save. After returning to the home page, you should see that text instead of the regular DSpace banner:In order to test the other metadata, you will have to not only repeat the steps for those metadata, but also make some changes in the code:
cris.cms.footerrequires you to editshowTopFooterinFooterComponenttotrue.cris.cms.home-newsrequires you to remove thecris.cms.home-headermetadata and then setcris.cms.home-newsmetadata. Or you can simply temporarily pull theds-home-newscomponent out of the@ifstatement in thehome-page.component.htmlfile.User Agreement edit
In order to test User Agreement editing, you have to select the
Edit User Agreementoption from the admin panel. You should see a page that will display all available languages and textareas for them. Find the language you have currently selected. For example, I set it for English:Then, you want to scroll down and press

Save. You will have two options available:There are two options here:
No, update only, which will save the metadata and do nothing else. Then you will need to create a new account and check if the user license content that is displayed to new users has changed.Yes, update and force, which will save the metadata and force all users to accept it again. You can use this and then you have to refresh the page to see the effect.In the end, you should see this:

Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.