Skip to content

Conversation

@pcg-kk
Copy link
Contributor

@pcg-kk pcg-kk commented May 24, 2024

feat: provide a language support for community pages

References

Add references/links to any related issues or PRs. These may include:

Description

In this PR we provide an mechanism to upload more than one language for communities and collections pages. User can define a page in all supported by DSpace languages from an customer environment file.

Instructions for Reviewers

Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.

List of changes in this PR:

  • We provide an form to save multiple languages
  • We changed a way to display texts from specific languages
  • We modify the backend API to save more than one metadata for each type

Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • 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.

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 3ba78b7 to d4bd7a5 Compare May 27, 2024 18:04
@pcg-kk pcg-kk marked this pull request as ready for review May 27, 2024 18:06
@tdonohue tdonohue added i18n / l10n Internationalisation and localisation, related to message catalogs new feature labels May 28, 2024
@tdonohue tdonohue requested a review from artlowel August 8, 2024 14:37
@github-actions
Copy link

github-actions bot commented Aug 8, 2024

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

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 4610aa1 to 88345f5 Compare August 13, 2024 17:29
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @pcg-kk!

This works well. I like the decision of putting the translation form next to the original, that makes it very easy to use!

This PR does change the behavior for languages that aren't (entirely) translated though. Currently it would fall back to the default language for fields that don't have a value in the current language. With this PR, fields that don't have a value in the current language are omitted. It would be good to try and retain the original behavior.

I also have a few minor inline comments.

<!-- Introductory text -->
<ds-comcol-page-content
[content]="collection.introductoryText"
[content]="collection.introductoryText(translateService.currentLang)"
Copy link
Member

Choose a reason for hiding this comment

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

I'd create a local property in the component to store translateService.currentLang, and initialize it in ngOnInit that will likely be more efficient than accessing the service from the HTML. This goes for all the components in this PR that use translateService.currentLang from the HTML

Copy link
Contributor Author

@pcg-kk pcg-kk Feb 10, 2025

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @pcg-kk, but I see it's still used in a bunch of other components. I'd do a search in your IDE for "translateservice.currentlang" in HTML files, and move it to the ts file for all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx for explanation - fixed

Copy link
Contributor

@minurmin minurmin Feb 9, 2025

Choose a reason for hiding this comment

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

The line

[content]="collection.copyrightText"

should include (translateService.currentLang) (as in community-page.component.html, https://github.com/DSpace/dspace-angular/pull/3078/files#diff-df2430517ff5adc042caccd6792890e6070e1f71221fee6c4fc42c5acfce299dR35 )

Otherwise a text copyrightText(S){return this.firstMetadataValue("dc.rights",{language:S})} would appear at the end of collection page.

this.defaultLanguageCode = environment.defaultLanguage;
this.defaultLanguage = environment.languages.find(lang => lang.code === this.defaultLanguageCode).label;

this.languages = environment.languages.filter(lang => lang.code !== this.defaultLanguageCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding only active languages to limit the length of the list. E.g.
this.languages = environment.languages.filter(lang => ((lang.code !== this.defaultLanguageCode) && (lang.active === true)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx - good remark. Fixed

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 095747a to 1348c1d Compare February 10, 2025 21:08
@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 1348c1d to cbd44c8 Compare February 10, 2025 21:17
@pcg-kk
Copy link
Contributor Author

pcg-kk commented Feb 10, 2025

@minurmin , @artlowel thanks a lot for review. I updated the repository to be sync with current main and applied your suggestions.

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 06d67ab to 9518554 Compare February 10, 2025 21:31
} else {
formMetadata[fieldModel.name] = [value];
}
const operations: Map<string, ReplaceOperation<{value: string, language: string}[]>> = new Map();
Copy link
Contributor

@minurmin minurmin Feb 19, 2025

Choose a reason for hiding this comment

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

This function (and more specifically, the blocks starting here and ending to submitForm.emit) is the only place in this PR that depends on DSpace/DSpace#9610. Based on discussion in server-side PR, you might want to consider implementing this based on PATCH ADD so REST Contract would not have to be changed. For example, something like this (additional checks are probably needed and there is room for optimization as I have limited experience with TypeScript):

const operations: Map<string, AddOperation<{value: string, language: string}>[]> = new Map();

(slightly different form of array because Operations are now iterated instead of values)

if ( keyExistAndAtLeastOneValueIsNotNull) {
  if (operations.has(key)) {
    const fieldOperations: Operation[] = operations.get(key);
    fieldOperations.push({
      op: 'add',
      path: key,
      value: {
        value: fieldModel.value as string,
        language
      }
    });
  } else {
    operations.set(key, [
      {op: 'add',
      path: key,
      value: {
        value: fieldModel.value as string,
        language
      },
    }]);
  }
}

(construct an array of add operations instead of array of values)

let finalOperations: Operation[] = [];
for (let [key,value] of operations) {
  finalOperations.push({op: 'remove', path: key}),
  finalOperations = finalOperations.concat(value);      
};

(before submitForm.emit construct another array that is suitable as such for the emit method instead of operations.values() and includes remove operation to ensure that old fields are cleared)

@pcg-kk pcg-kk requested a review from artlowel March 6, 2025 20:39
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @pcg-kk!

This looks good to me now, but please still take a look at @minurmin's comment about DSpace/DSpace#9610. If it's possible to do this without the additional REST PR that would be ideal

@minurmin
Copy link
Contributor

minurmin commented Mar 7, 2025

A word of warning: current implementation of PATCH add is not quite in line with the REST contract (see details in review related to DSpace/RestContract#304 ). I.e. at the present you can't replace values with plain add but existing values must be cleared first (as demonstrated in my previous comment). As a result, the implementation becomes slightly more complex (at least until https://github.com/DSpace/DSpace/blob/main/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataAddOperation.java code is fixed) compared to proposed PATCH replace. But it's certainly possible to do (adaptation of this PR to DSpace 7 is in use at the University of Jyväskylä).

@pcg-kk
Copy link
Contributor Author

pcg-kk commented Mar 7, 2025

We analyze it yesterday, and still have some other problems with languages. We will try again on Monday.

@github-actions
Copy link

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

@tdonohue tdonohue moved this from 👀 Under Review to ❓ Stalled/On Hold in DSpace 9.0 Release Mar 28, 2025
@tdonohue tdonohue moved this to 🙋 Needs Reviewers Assigned in DSpace 10.0 Release Apr 2, 2025
vins01-4science pushed a commit to 4Science/dspace-angular that referenced this pull request Apr 11, 2025
…e#3078)

Enhance metadata form handling for updated fields
@tdonohue tdonohue moved this from 🙋 Needs Reviewers Assigned to 👀 Under Review in DSpace 10.0 Release May 19, 2025
@minurmin
Copy link
Contributor

With DSpace/DSpace#11018 (should be easily mergeable to earlier DSpace versions as well) this should be implementable with less changes as mentioned in my earlier comment - possibly even by just changing PATCH call from replace to add.

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

Labels

i18n / l10n Internationalisation and localisation, related to message catalogs merge conflict new feature

Projects

Status: 👀 Under Review

Development

Successfully merging this pull request may close these issues.

Enabling translation of collection's/community's names and description

4 participants