pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
297 stars 443 forks source link

Site-wide fall-back for the privacy notice in Admin > Site Settings not displaying properly #6594

Closed ewhanson closed 3 years ago

ewhanson commented 3 years ago

Describe the bug When the journal-specific privacy message is removed and the site-wide privacy notice in place, the journal instead displays a message in Spanish rather than site-wide default.

To Reproduce Steps to reproduce the behavior:

  1. As an admin user, set a site-wide privacy statement under 'Administration' -> 'Site Settings' -> 'Site Setup' -> 'Information'
  2. Within the journal, remove the existing privacy statement for the specific journal under 'Website Settings' -> 'Setup' -> 'Information'.
  3. In the journal front end, go to 'About' -> 'Privacy Statement'
  4. See message in Spanish instead of site-wide default.

What application are you using? OJS 3.3rc

Additional information Screen Shot 2021-01-13 at 6 02 14 PM

NateWr commented 3 years ago

What are your language settings like and what is the es_ES value of the privacy statement set to in the journal/admin settings?

NateWr commented 3 years ago

Ok, I figured out what's happening. The privacy statement is falling back to the next localized value that it finds in the context setting. And localized values exist in all four languages installed on the site:

privacy-statement

These are installed when the context is created based on the supportedLocales of the site: https://github.com/pkp/pkp-lib/blob/master/classes/services/PKPContextService.inc.php#L327-L353. Since the form to add a new context does not include any language configuration, defaults will always be installed for all of a site's supported locales.

I suspect this same problem exists in 3.2.x.

@asmecher what's your take on this? Is there any justification to installing defaults for all locales supported by the site? Or do we need to make primaryLocale and supportedLocales required properties before a context can be created?

I'm leaning towards the second but I just wanted to make sure that I wasn't missing anything obvious... I suspect that this has always been the way that defaults worked.

NateWr commented 3 years ago

I went ahead and worked up some PRs that add fields for supportedLocales and primaryLocale when creating a new context. They also add some validation checks to ensure the selected locales are valid.

Does this sound like the right way to go?

PRs: https://github.com/pkp/pkp-lib/pull/6602 https://github.com/pkp/ojs/pull/2992 https://github.com/pkp/omp/pull/909 https://github.com/pkp/ops/pull/108

asmecher commented 3 years ago

@NateWr, as a more general solution, how do you feel about letting the multilingual controls appear for content in any language that is populated, rather than a subset? This issue is an example of a broader problem of "orphaned" locale data; consider when the admin/manager changes the locale configuration to exclude a locale that has data added. That data is left in the DB (and in some cases might be fetched as a fallback when other data isn't available) but will never appear in the UI.

NateWr commented 3 years ago

@asmecher I worry this would be a source of confusion. It would result in instances where things are being shown to users who have no idea why it's there or what they are meant to do with it. Authors and editors might see content in a publication that won't appear on the frontend. On large multi-tenant sites the journal defaults might exist in a bunch of languages, making it unclear which ones the journal actually supports. And in general, I think end users will think it's a bug when they see inputs for a language that a journal has not been configured to support.

If journals need to edit data from a disabled language, they can always enable the language to expose it and edit it.

I agree that having the orphaned data in the database is likely to lead to problems like this. We could build into the DAOs limits to what is retrieved from _settings tables, so that a submission dao would only retrieve from supportedSubmissionLocales and the data would never make it into the data objects. But that would involve some additional joins that would likely impact performance.

And I suspect there are cases out there where journals maybe used to publish submissions in two languages, but now only publish in one. But still support both languages in the reader-facing UI, so that old data is still important, even while it's important that for ongoing operations the other language is not scattered all over the editorial UI.

asmecher commented 3 years ago

We could build into the DAOs limits to what is retrieved... But that would involve some additional joins that would likely impact performance.

Agreed, and we'd have to scatter these limits through dozens of queries, likely missing some and with a good potential for regression.

I do think language configuration changes are more major than some administrators expect them to be, so perhaps better guidance (confirmations when changing configuration) would be an easy start.

OK, let's go ahead with your solution. And I have to say that I still think our language model is fundamentally good -- it's the diversity in deployments (and changing configurations) that makes this complicated.

NateWr commented 3 years ago

And I have to say that I still think our language model is fundamentally good -- it's the diversity in deployments (and changing configurations) that makes this complicated.

I agree. The complexity of multilingualism is under-appreciated and I think one of our unique advantages over other platforms. It's bound to be an uncomfortable fit in some cases, especially as journals evolve over time and add/drop support for languages (or publish one-off articles/issues in a new language).

Sorry, one more question came to mind. @asmecher should we try to clear out this default data in an update? In a multi-tentant system, any journals created since 3.2 will have a lot of default data for irrelevant languages. We could try to cull multilingual data for non-supported languages in context settings on a migration? Pros: this data is cleaned up. Cons: some potential for undesired data removal (but I think less critical than doing this on submission data).

NateWr commented 3 years ago

All merged. During upgrade we'll clean out data for unused locales. I've added a note on this to the release notebook.