Change submission language

[jyhein]

• Change the submission language after submission
• Show the language of the submission in the workflow page and the author dashboard
• In the modal window editors can change the language and add/edit required metadata
• The submission's metadata can be edited even if the submission's locale is not one of the currently supported submission locales
• The language of the submission can not be changed if the status of the publication is published or there are more than one version

[bozana]

Hi @jardakotesovec, could you please take a look at this PR?
Thanks a lot!

[jardakotesovec]

Hi Jyrki,
you are on the good track - but hopefully we will be able to simplify things.

I added some suggestions how to proceed - feel free to follow up with questions either on github or mattermost.

[jardakotesovec]

Number of props after the adjustments should hopefully drop significantly.

There is good benefit to have the modals as independent as possible from main screen and keep the communication between them at the minimum - as that makes easier for both to evolve independently.

If we manage to move the updating meta to the API endpoint, instead of handling the whole process from the UI - I think we will get just to passing the cslmform and submission. It could be even more independent and just pass submissionId & cslmform and fetch the submission from the API.

[jardakotesovec]

With new composition API I created couple of composables to make various things easier. Please follow the useFetch examples here - https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/composables-usefetch--docs

It will handle many things for you, things like passing csrf token and adding the x-http-method-override logic and error handling for unexpected errors.

[jardakotesovec]

One more thing - we also now have useForm composable - but its not covered in storybook yet, because its still in early stages, but could be useful for your use case.

Purpose is basically to stop writing code which is traversing the form structures - and have that abstracted that out.

So for example if you would want to programtically set some values, you can do something like:

const { form, set, setValue } = useForm(props.changeLanguageForm);

setValue('abstract', abstract);

And you can use the set to process the changes coming from form:

<pkp-form
  v-bind="form"
  @set="set"
/>

As mentioned its early implementation - so if you run into any issues there please let me know and I will improve it..

[jardakotesovec]

@jyhein I see there is some good progress - let me know when its ready for review. Thank you.

[ewhanson]

No problem with this, but just wondering if this was something that had been breaking the change submission locale functionality in some way or was it a separate bug you caught while working on it?

[jardakotesovec]

Hi @jyhein, Could you provide some details on this one? Also not sure why this was needed and whether its related to locale functionality.

[jyhein]

I got an error that the value is expected to be boolean but that was when vue side updated contributor props. Not sure if if it is needed anymore (or at all)

[jardakotesovec]

@jyhein Oki, can you please double check whether is needed?

[jyhein]

I removed the code because I didn't see any errors OMP side when changing language. I didn't test any further

[ewhanson]

I would specify which API URL this is since it's used both as the publication endpoint and as part of the changeLocale endpoint. Maybe something like publicationApiUrl as distinct from changeLocaleApiUrl?

[jardakotesovec]

Rather than destructing 'value'. More common approach with composition api, is to keep it as ref and when you need to access value, you do it like: form.value.action = apiUrl.value + '/changeLocale' and as Erik mentioned destructing it like {apiUrl: publicationApiUrl} would make it bit more friendly.

[jyhein]

I wanted to make apiUrl (publicationApiUrl) immutable. No other reason

[ewhanson]

Is the publication title guaranteed to be index 2 in the props.form.fields array? This may be totally fine but just wanted to flag it.

[jardakotesovec]

Using publication title from the form is bit tricky, because it gets loaded on page load. But in case editor changes that in the form it would show the old one, right?

What about using the title that you get from the publication endpoint?

[ewhanson]

This looks fine, but just wanted to double check (cc: @jardakotesovec) that an IIFE was beset practice for preloading this data here as I'm less familiar with working with stores.

[jardakotesovec]

@ewhanson @jyhein Its bit unusual, not technically wrong though :-). Did you have any reason to wrap it in function like this? Maybe just to avoid variable name conflicts? That could be avoided for example with const {data: publication, fetch: fetchPublication} = useFetch(..).

Or if you would want to keep it in function, I would suggest just to create function with name and than invoke it.

[jyhein]

Vue complained something about making <script setup> async (.vue file), so I had to wrap it in a function

[ewhanson]

Another minor one, but for readability, I prefer having more explicit variable names as they make it easier to parse the code at a glance (maybe field instead of f?).

[ewhanson]

Probably another question for @jardakotesovec on house style. Elsewhere in the codebase, the window is explicitly referenced (so window.location.reload() instead of just location.reload()). Should we keep it consistent with the rest of the codebase and include window or is it better to leave it off?

[jardakotesovec]

This is very minor thing, but yes we can keep it consistent with rest of the code base and add window

[jardakotesovec]

My understanding here is that you want to extend set function, so if the language gets changed it also updates title and abstract, right?

To just to make it bit more explicit that you are extending default function. You can get the set function from useForm - const {set} = useForm(...). And than create your own function setCustom, which calls the original set function and do your custom bit.

[jardakotesovec]

Yes, locale keys are being determined in build time, therefore it can't really understand variables.

But in this case you are always updating title and abstract, so instead of loop, you can just always apply it for title and abstract.

Or if you want to stick with the loop, you can define the locale keys for example like this:

const DescriptionLocaleKeys = {
title: tk`submission.list.changeSubmissionLanguage.metadataDescription.title`,
abstract: tk`submission.list.changeSubmissionLanguage.metadataDescription.abstract`
}

Which than can be used as t(DescriptionLocaleKeys['title'], {language: ...})

Here is docs for use of tk - https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/composables-uselocalize--docs#tk---translation-keys

Let me know if there is any issue with this.

[jyhein]

My idea here was to keep the code as general as possible, in case there will be additional fields

[jardakotesovec]

Thats good intention for sure, I have only mixed feeling always when I see some string replacement as that feels fragile. I know its very likely to be fine, but theoretically there can be some scenario where the locale name could be substring of some word in the description in some specific language. Its unlikely, but that still would make me to prefer to use the locale keys as suggested above.

[jyhein]

Ok

[jardakotesovec]

@jyhein Hi Jyrki, I am done with code review, apologies for delay. Its looking good. Its all really just minor suggestions.