Enhance FileSizePipe to support localization

[saschaszott]

Fixes #5054 .

[MMilosz]

I'm wondering if we should stay with JEDEC (e.g., 1 MB = 1024 KB) or SI (e.g., 1 MB = 1000 KB). Even though I'm personally all for JEDEC, I believe most people in the world are familiar with the SI and expect SI units

Or maybe it should be moved to the configuration, so users could decide by themselves?

[saschaszott]

@MMilosz , could you please create a new issue for this aspect.

[tdonohue]

@saschaszott : This PR looks to be invalid, as it contains a large number of unrelated commits. Did you maybe submit this to the wrong branch? (I see the branch you are submitting against is set to dspace-8_x. Maybe it should be main?)

[saschaszott]

@tdonohue , I originally implemented the extension of the FileSizePipe for DSpace CRIS 2024.02.03, which is based on DSpace 8.2. It took a few iterations to make it work with DSpace 10 as well. It should be working now. If we want to backport this change to older DSpace versions, the code will need to be adapted.

[saschaszott]

I unfortunately have to pause at this point, as I currently don’t have a DSpace 10 development instance available. There seems to be an issue with the dependency injection of LocaleService into the FileSizePipe. Perhaps someone else can help? Alternatively, I can create a new PR to integrate this functionality into DSpace 8.

[saschaszott]

I have now set up a local DSpace Angular 10 test instance and applied the change there. In Chrome, I don’t see any error messages, and the desired result (localized display of the file size) is also visible. Therefore, the build error must have other causes.

[saschaszott]

I think I have found the cause: the LocaleService (or a mock of it) needs to be provided as a provider in the spec tests.

[saschaszott]

@tdonohue , the PR is now finished; all spec tests are passing again. Should I create a new PR with one or two commits containing all changes, or will you squash all commits when merging?

Also, there is currently a general issue with running the E2E tests on GitHub (see my question on Slack).

[tdonohue]

@saschaszott : If you have a chance, I'd prefer you squash the commits yourself in this PR. This is possible to do by just doing a rebase on the branch this PR is based on (to squash the commits however you like) and do a "force push" to update the branch in this PR.

That preferable just because I sometimes forget to squash PRs when merging them.

[saschaszott]

@tdonohue , this PR is ready for review!

[MMilosz]

I tested this locally and everything works fine. 👍 I checked various file sizes between 0B-1MB, locales (en, de, fr, fa).

English	French

German & Persian screenshots

German (uses comma)

Persian (uses momayyez, ٫)

[tdonohue]

👍 Thanks @saschaszott ! The code looks good and I also gave it a test. Works well. Because this is a tiny change and relates to i18n, I'm going to attempt to auto-backport this to 9.x as well.

[dspace-bot]

Backport failed for dspace-9_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-9_x
git worktree add -d .worktree/backport-5055-to-dspace-9_x origin/dspace-9_x
cd .worktree/backport-5055-to-dspace-9_x
git switch --create backport-5055-to-dspace-9_x
git cherry-pick -x d6af4addca63e3c3620d04f0b1cdcf94865ecb52

[tdonohue]

The backport failed. If someone has an interest in backporting this to 9.x, I think we could consider this a "bug fix" to i18n.

(I wasn't planning to backport it further though because this area of the code is not quite the same in older releases)

[saschaszott]

@tdonohue , I can create a backport of this PR for DSpace version 7–9. Should I open a new PR for that?

[tdonohue]

@saschaszott : Yes, please open a new PR for backporting this to earlier versions. At a minimum, it'd be nice to backport this to 9.x. But, if we can backport it further, that's fine with me.