Fix autodiscovery Opensearch tag

[J4bbi]

References

• Fixes https://github.com/DSpace/dspace-angular/issues/791
• Requires https://github.com/DSpace/DSpace/pull/9058

Description

The RSS component injects link tags into the document header. One of them is for autodiscovery and is incorrectly formatted (as explained in the issue).

This PR

• changes the value for the href and type attributes of the autodiscovery tag (lines 97-98)
• reformats the code to improve readability
• makes minor changes to documentation

Instructions for Reviewers

You need to add two variables to the whitelist of exposed variables in the REST API. See DSpace PR 9058. Open up the main landing page and inspect the html.

Where it said: <link href="https://sandbox.dspace.org/server/opensearch/search/service" type="application/atom+xml" rel="search" title="Dspace">

it will now read: <link href="https://sandbox.dspace.org/server/opensearch/service" type="application/opensearchdescription+xml" rel="search" title="Dspace">

For consideration:

• According to the spec, the value for the title attribute, which I have left unchanged is currently "Dspace" (and should probably by DSpace, with a capital S?) but " may contain a human-readable plain text string describing the search engine." It would be possible to fetch a value from config such as dspace.name.
• I've hardcoded the URL to be environment.rest.baseUrl + '/opensearch/service', I'm not confident this should not be configurable under Opensearch configuration settings

[J4bbi]

Having another look at the config.

the title attribute should probably be set as websvc.opensearch.shortname

[alanorth]

Ah, good catch @J4bbi! Before, my site was adding the incorrect link:

<link href="http://localhost:8080/server/opensearch/search/service" type="application/atom+xml" rel="search" title="Dspace">

After, the link is correct:

<link href="http://localhost:8080/server/opensearch/service" type="application/opensearchdescription+xml" rel="search" title="Dspace">

And I was able to see that Firefox now detects the OpenSearch endpoint and offers to add my repository as a search engine when I type in the URL bar.

Having another look at the config. the title attribute should probably be set as websvc.opensearch.shortname

I agree. In fact, looking at my DSpace 6 instance, I see that the OpenSearch functionality sets both the short and long names.

[J4bbi]

I'm converting this to draft as it is apparent that a parallel PR for the backend is required to expose more opensearch configuration variables.

There are also other issues I'll go into in more detail when I have time tomorrow or later in the week.

[J4bbi]

The comment that was here has been created as a separate backend issue DSpace#9056.

[alanorth]

Thanks @J4bbi. I've applied this patch along with https://github.com/DSpace/DSpace/pull/9058, but I still see "DSpace" instead of my site's name in the OpenSearch head tags.

I have verified that the backend correctly exposes websvc.opensearch.shortname with my site name. I tried shift refreshing...

[J4bbi]

I've now gone away from trying to set variables in the component's outer scope from an inner scope ca22491

Over to using combineLatest, which if I understand correctly combines getting four configuration properties along with pagination search options, to action on all of them at once.

All of the action therefore happens within the scope of that combined subscription as I understand it (L64-100).

I am not certain I am now doing things the right way, now that switchMap is not being used.

I readily admit I don't have a good understanding of how Angular works at this level.

Changes to code behaviour from the earlier version are:

• code stops executing if websvc.opensearch.enable is not "true" (L81-83)
• code now respect websvc.opensearch.autolink , as to whether a link tag is inserted for autodiscovery
• code now respects websvc.opensearch.shortname to be the description of the service for autodiscovery

Potential problems

• is the code resilient for low or no latency, what if these configuration settings are commented out completely?
• as I do not fully understand switchMap functionality I can not say if the code works as before
• I don't fully understand the ondestroy code (L52-57). A when does that happen? Presumably the RSS component will not be toggled on and off? B is it necessary to unsubscribe from all subscriptions? my understanding is that some only live to deliver a single value and then self-unsubscribe? C the code indiscriminately removes all tags with rel='alternate', is that definitely safe? If it is cleaning up after itself, this component ejects 2 or 3 link tags into the header, two of which will have rel='alternate' but the optional third, the autodiscovery tag, has rel='search'.

[artlowel]

• is the code resilient for low or no latency, what if these configuration settings are commented out completely?

I'd urge you to write tests for those cases in the spec file. If you use observableOf to mock the output of findByPropertyName (or more likely createSuccessfulRemoteDataObject$ since you need a RemoteData, but that uses observableOf internally) it will emit without latency, so you can verify what would happen.

as I do not fully understand switchMap functionality I can not say if the code works as before

A switchMap maps the source to another observable. Every time the source observable changes, it will unsubscribe from the previous mapped observable, and subscribe to the new one.

In this example openSearchUri comes from the source observable, and is used to do another call that results in an observable: that this.searchConfigurationService.paginatedSearchOption call. So if the source observable were to emit an updated openSearchUri value, switchMap would do a new call using that value, and stop emitting responses from the call using the old URI. That's as a opposed to mergeMap, which would simply keep emitting all values from all previous calls combined. In this case the difference doesn't matter as the source observable will only emit once anyway. But in most cases you're only interested in the most recent data, so you need a switchMap when turning one observable into another. That's probably why the original dev chose it.

I don't fully understand the ondestroy code (L52-57). A when does that happen? Presumably the RSS component will not be toggled on and off?

ngOnDestroy is the cleanup method for a component. It will be executed whenever that component stops being rendered, so in this case probably when you navigate from a page that has an RSS component, to a page that doesn't

B is it necessary to unsubscribe from all subscriptions? my understanding is that some only live to deliver a single value and then self-unsubscribe?

Strictly necessary no, but it's good practice. Because even if an observable will only emit once and then complete, if for some reason it never emits, the subscription remains open, in browser memory, indefinitely

C the code indiscriminately removes all tags with rel='alternate', is that definitely safe? If it is cleaning up after itself, this component ejects 2 or 3 link tags into the header, two of which will have rel='alternate' but the optional third, the autodiscovery tag, has rel='search'.

No, that's not safe. The better solution would be to give the tags added by this component a unique class, that way we can remove only the ones with that class at the end. This is the way ThemeService does it

[github-actions[bot]]

Hi @J4bbi, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!