Fix match theme by handle with canonical prefix https://hdl.handle.net/ not working

[alexandrevryghem]

References

• Fixes Bug: theme matching by handle doesn't work, but uuid does #2348
• Backend PR Fix match theme by handle with cannonical prefix https://hdl.handle.net/ not working DSpace#9162

Description

This PR fixes a problem with this regex that assumes all handle URLs are prefixed by a handle/. This is not the case if you set the handle.canonical.prefix to https://hdl.handle.net/. The solution is to retrieve the handle.canonical.prefix from the backend and use it inside the regex. By default the handle.canonical.prefix is not exposed, so you also need the backend PR in order to test this fix.

Instructions for Reviewers

List of changes in this PR:

• The HandleService now retrieves the handle.canonical.prefix from the backend. This means that normalizeHandle doesn't return a string | null anymore but an Observable<string | null>. This means that the code using this method also needs to be refactored:
  • The CurationFormComponent used this method, and it was fairly simple to switch to Observables.
  • The HandleTheme also uses normalizeHandle in its matches method. So this means that the return type of the matches method needs to be changed for all the classes extending Theme from boolean ➜ Observable<boolean>, but this also means that more methods needed to be refactored on the two places where this method is used:
    • In the updateThemeOnRouteChange$ method, the code that finds which theme should be used when you're not on a Community/Collection/Item page now still has to iterate over the list of available themes and return the first theme where its matches method returns true. The only difference is that it now returns an Observable<true> instead, so I had to wrap everything in a concatMap to ensure that even if the second theme returns true faster than the first theme, the first matching theme is still returned. Luckily this part of the code was already in a switchMap, so this refactor cascade stops here.
    • The matchThemeToDSOs also uses Theme.matches() and had to be refactored in the same way as above. But it not only had to iterate over all the themes but for each theme, it also had to iterate over the DSO you are currently on, but also the parent DSOs. For those DSO-theme combinations, it had to use Theme.matches() and now also had to always return the first matching theme no matter the order in which the Observables resolve. This method was only used in the updateThemeOnRouteChange$ method inside the switchMap, so the refactor cascade stops here.
• I also noticed that an error is sometimes thrown in CommunityPageSubCommunityListComponent & CommunityPageSubCollectionListComponent because the destroy is sometimes called before the component has time to fully initialize. This can happen when the component initializes itself in the default theme, but then while initializing, a match is found for a UUIDTheme/HandleTheme/RegexTheme so the component is destroyed before the config property could be initialized.

Guidance for how to test/review this PR:

• To test this, we need 2 themes. You can use the 2 existing default themes for this: custom & dspace
  • Uncomment the CustomEagerThemeModule in src/themes/eager-themes.module.ts
  • Make a change to a part of the page that is used on every page like the HeaderComponent (this will make testing easier). By default you can see that the dspace theme uses a green bottom border, while the custom theme uses the base theme's header by default without that green border.
• Set the handle.canonical.prefix = https://hdl.handle.net/ and ensure that rest.properties.exposed = handle.canonical.prefix is also present in your config.
• Make a new community/collection/item after rebuilding your backend. They should now have https://hdl.handle.net/ in their handle URL.
• Add that handle to the custom theme in your config.yml like this:

  themes:
  - name: custom
    handle: 123456789/8
  - name: dspace

• Verify that it now works correctly when you go to https://hdl.handle.net/123456789/8 for example, but check that the theme is also applied to all the child DSOs (meaning that if you put the handle of the community in your config.yml, the same theme should also be applied to all its sub-communities, collections & items).
• Verify that other theme config like headTags also still work like expected because a lot of changes were made.

Checklist

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using yarn lint
• My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

[tdonohue]

@alexandrevryghem : Gave this a review/test today (along with the backend PR). Overall, it looks good and it works as described. I've tested all the various theme configurations, including handle (both for hdl.handle.net and dspace.ui based), regex, and uuid. They all work well for me.

However, when running in Production mode (yarn build:prod; yarn serve:ssr), I'm seeing errors in the DevTools console of my browser. These seem to be caused by this PR as they don't occur on main. The console errors look like this:

main.b3ecb47baf1f06fa.js:1 ERROR TypeError: Cannot read properties of undefined (reading 'responseMsToLive')
    at main.b3ecb47baf1f06fa.js:1:224248
    at main.b3ecb47baf1f06fa.js:1:2860890
    at a._next (main.b3ecb47baf1f06fa.js:1:2856910)
    at a.next (main.b3ecb47baf1f06fa.js:1:2845710)
    at main.b3ecb47baf1f06fa.js:1:2859619
    at a._next (main.b3ecb47baf1f06fa.js:1:2856910)
    at a.next (main.b3ecb47baf1f06fa.js:1:2845710)
    at main.b3ecb47baf1f06fa.js:1:2850375
    at a._next (main.b3ecb47baf1f06fa.js:1:2856910)
    at a.next (main.b3ecb47baf1f06fa.js:1:2845710)

This error occurs twice for me immediately on loading the homepage. It then continues to occur on other page loads as well. I haven't dug deeper than that, but obviously we'd need to find a fix before we could merge this. If you are able to find a fix in the next day or two, there's still time to get this into 7.6.1. But, ideally we'd get this merged by end of day Monday at the latest.

[alexandrevryghem]

@tdonohue, it was a simple mistake this take(1) used to be called before the getFirstCompletedRemoteData()

[tdonohue]

@alexandrevryghem : I'm testing this again right now, but still seeing the same errors in production mode (yarn build:prod; yarn serve:ssr). Did you push the fix up? I've tried rebuilding (in case something was cached). But, each time I'm still seeing those same "TypeError" errors in my browser's console when I first access the homepage.

[tdonohue]

@alexandrevryghem : Set this aside for a while and came back to it. It appears something was cached previously (maybe in my browser?). I'm not seeing the console error anymore. I'll do some further testing today to make sure it doesn't reappear and then (hopefully) get this merged. Thanks!

[alexandrevryghem]

@tdonohue, I also switched from to a different branch since yesterday & ran a yarn clean and now I can also reproduce this error again, I will look into it 🤔

[tdonohue]

@alexandrevryghem : Is this ready for re-testing? Or is the prior error still being looked into? Thanks!

[alexandrevryghem]

@tdonohue it should be fixed 🤞

[tdonohue]

👍 Thanks @alexandrevryghem ! Retested this today, and have been running this for the last hour & not noticed any errors anymore.

Also tested again that theme configs via handle (both local URL & hdl.handle.net), regex and uuid all work perfectly. So, this fixes the bug and doesn't appear to have any new side effects.

[dspace-bot]

Successfully created backport PR for dspace-7_x:

• [Port dspace-7_x] Fix match theme by handle with canonical prefix https://hdl.handle.net/ not working #2636