Skip to content

[DURACOM-339] fix downloads not working when matomo is disabled#4125

Merged
tdonohue merged 1 commit intoDSpace:mainfrom
4Science:task/main/DURACOM-339
Apr 4, 2025
Merged

[DURACOM-339] fix downloads not working when matomo is disabled#4125
tdonohue merged 1 commit intoDSpace:mainfrom
4Science:task/main/DURACOM-339

Conversation

@AndreaBarbasso
Copy link
Contributor

References

Description

Fixed the bug where downloads were not working when matomo is disabled.

Instructions for Reviewers

I inserted a check for the matomo.enabled configuration setting before using the matomoService to add the visitorId (that's where the download was failing).

List of changes in this PR:

  • Added a check for matomo.enabled in BitstreamDownloadPageComponent to avoid calling a matomoService method is matomo is disabled;
  • Added tests.

Include guidance for how to test or review your PR.
You can easily check that any download now starts when matomo is disabled, and still works when matomo is enabled.

Checklist

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • 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 npm run lint
  • My PR doesn't introduce circular dependencies (verified via npm run 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.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • 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 tdonohue added bug high priority 1 APPROVAL pull request only requires a single approval to merge labels Apr 3, 2025
@tdonohue tdonohue moved this to 🙋 Needs Reviewers Assigned in DSpace 9.0 Release Apr 3, 2025
@tdonohue tdonohue requested review from kshepherd and tdonohue April 3, 2025 19:48
@tdonohue
Copy link
Member

tdonohue commented Apr 3, 2025

@AndreaBarbasso : Thanks for this early PR. I wanted to note that I gave it a quick code review and test today. The code looks good. I've also verified the following:

  1. The download error is fixed. When Matomo is disabled, downloads are now occurring properly if I point this PR at the Sandbox backend (https://sandbox.dspace.org/server/) or if I use a backend running on localhost.
  2. The matomo.js file is still being downloaded when Matomo is disabled. (You mentioned this in the ticket though)

One brainstorm about avoiding the matomo.js download. Is there any way to "lazy load" matomo only when it's needed? I know we do that with Orejime (and Klaro in 7.x/8.x) here: https://github.com/DSpace/dspace-angular/blob/main/src/app/shared/cookies/browser-orejime.service.ts#L70-L79 I'm not sure if there's a way to just avoid importing Matomo altogether when it's disabled? (I honestly haven't looked into it closely, but I wanted to call mention this in case it helps stimulate other ideas.)

@tdonohue
Copy link
Member

tdonohue commented Apr 3, 2025

@AndreaBarbasso : One last thing to note. If you find that the bug related to downloading matomo.js is not easy to solve, then I'd recommend we split this into two tickets:

  1. Leave this PR as-is, because it's good enough to fix the severe issue in Downloads are broken in main branch #4122
  2. Create a new ticket to describe the matomo.js download issue.

That's perfectly OK if we have to go that route. Mostly, we just must fix the severe error with downloading files before the Testathon. The issues with matomo.js downloading can always be fixed up during Testathon if we need more time.

@AndreaBarbasso
Copy link
Contributor Author

@tdonohue, matomo is already lazy loaded with the mode: 'deferred' option in the provider, but it seems that it still applies under the hood the <script> tag that downloads matomo.js.
I tried some quick changes, hoping to pin down the issue and solve it, but it seems harder than we thought.
So, I'm absolutely positive towards your suggestion to split this into two tickets, and work at the second issue during Testathon!

I'm opening this PR - thanks for your help!

@AndreaBarbasso AndreaBarbasso marked this pull request as ready for review April 4, 2025 08:25
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @AndreaBarbasso ! This looks good to me then and fixes the severe bug. I'll move the other issue to a separate ticket & assign it to you to work on during Testathon.

@github-project-automation github-project-automation bot moved this from 🙋 Needs Reviewers Assigned to 👍 Reviewer Approved in DSpace 9.0 Release Apr 4, 2025
@tdonohue tdonohue added this to the 9.0 milestone Apr 4, 2025
@tdonohue tdonohue merged commit de44252 into DSpace:main Apr 4, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from 👍 Reviewer Approved to ✅ Done in DSpace 9.0 Release Apr 4, 2025
@tdonohue
Copy link
Member

tdonohue commented Apr 4, 2025

Other issue was moved to #4128

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug component: statistics high priority

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Downloads are broken in main branch

2 participants