Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed translation cache access when the translation content is changed #208

Closed

Conversation

gushil
Copy link

@gushil gushil commented Jul 18, 2024

Closes #

The problem occured when editing the form (that already submitted before) and the page with multiple selectpickers in repeating group is always showing the wrong select text.

From my observation, it is caused by selectpicker using i18n translation with i18nNumber involved.
We will have same translations for all selectpicker widgets because all widgets have same i18n data eventhough it has different i18nNumber.

This select instances

<group_nm7ec42  enk:ordinal="1" >
	<multiselect2>a b c</multiselect2>
</group_nm7ec42>
<group_nm7ec42  enk:ordinal="2" >
	<multiselect2>a b</multiselect2>
</group_nm7ec42>

will give us
image

It is supposed to give us
image

I have verified this PR works with

  • Online form submission
  • Offline form submission
  • Saving offline drafts
  • Loading offline drafts
  • Editing submissions
  • Form preview
  • None of the above

What else has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

The solution that minimize cache key access, translation call and just adding one string comparison.
Already tried the solultion in selectpicker but that is not an optimal one.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I don't think there is any. All tests are passed.

Do we need any specific form for testing your changes? If so, please attach one.

No.

@gushil gushil requested a review from MartijnR July 18, 2024 10:20
Copy link
Member

@MartijnR MartijnR left a comment

Choose a reason for hiding this comment

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

Hi Agus,

I was actually not going to review this (because I only agreed to review the filename issue) and just suggest you create a PR for enketo/enketo since it looks like this is not related to an OpenClinica customization.

However, I couldn't help peaking inside, and made a comment after all...

Still, I'd recommend continuing this work in enketo/enketo, and wait for somebody there to review it. Of course, you'd have to first confirm that the issue indeed occurs in enketo/enketo and perhaps add an issue for it there. The BIG advantage for yourself is that if they implement their own fix some day, it would create a painful merge conflict. Moreover, it is also the right thing to do, in my opinion.

packages/enketo-express/public/js/src/module/translator.js Outdated Show resolved Hide resolved
@gushil gushil requested a review from MartijnR July 26, 2024 12:58
Copy link
Member

@MartijnR MartijnR left a comment

Choose a reason for hiding this comment

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

Looks good with a chance to simplify further.

I only looked at the code, didn't try to reproduce the bug or test the fix.

I recommend offering the next version to enketo/enketo for their review and also file a bug report there. I think this would be a welcome fix for a bug! 👍

translationKey.length -
el.dataset.i18nNumber.toString().length -
1
);
Copy link
Member

Choose a reason for hiding this comment

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

Is the translationKey variable actually necessary? This might just be el.dataset.i18n.

@gushil
Copy link
Author

gushil commented Aug 1, 2024

Created PR in upstream (enketo/enketo) to fix this issue.

enketo#1336

Thanks.

@gushil gushil closed this Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants