-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
SF-3162 Warn user when training and drafting sources are different #2982
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2982 +/- ##
=======================================
Coverage 82.54% 82.55%
=======================================
Files 551 553 +2
Lines 31965 31980 +15
Branches 5181 5174 -7
=======================================
+ Hits 26387 26401 +14
- Misses 4809 4811 +2
+ Partials 769 768 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Overall it looks good. I like the languageCodesEquivalent
solution, it's simple and should cover the majority if not all cases we would see.
Rather than showing an additional warning message, what if we only show the "All languages are correct" notice if we determine there is a mismatch between training and drafting source languages? We could change the required languagesVerified
message to "I understand, proceed drafting with selected languages".
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.ts
line 88 at r1 (raw file):
private sourceLanguagesAreCompatible(source: TranslateSource, other: TranslateSource): boolean { if (source.writingSystem.tag === other.writingSystem.tag) return true; return this.i18nService.languageCodesEquivalent(source.writingSystem.tag, other.writingSystem.tag);
nit I wonder if this should take a source[]
that we loop through and compare source[0]
to each item? Doing this now should prevent us from having to update if/when we go to more than 1 alternate source.
Code quote:
private sourceLanguagesAreCompatible(source: TranslateSource, other: TranslateSource): boolean {
if (source.writingSystem.tag === other.writingSystem.tag) return true;
return this.i18nService.languageCodesEquivalent(source.writingSystem.tag, other.writingSystem.tag);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the "language confirmation" notice, the existing direction is to have the notice showing all the time with the hopes that the user checks and confirms the languages every time, as a safeguard. @Nateowami may have more to say on this.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.html
line 55 at r1 (raw file):
@if (showSourceLanguagesNotCompatibleError) { <app-notice type="error"> <span>{{ t("sources_must_be_same_language") }}</span>
If there are multiple training sources and their languages differ, we show a message on the main drafting page and prevent them from even getting to the first step. I believe we should follow this pattern, unless there's a good reason not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR changes that requirement. We've confirmed with the Serval/EITL teams that if we verify the language codes are equivalent then draft quality will not be degraded.
I guess the question on keeping it as a safeguard for all instances would be if we believe selected sources have a high probability of having an incorrect language code set that needs reviewed before proceeding? Otherwise, I'd suggest that only displaying it when we detect a different language code alerts the user draft quality would be reduced but still allow them the option to proceed.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @josephmyers and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.html
line 55 at r1 (raw file):
Previously, josephmyers wrote…
If there are multiple training sources and their languages differ, we show a message on the main drafting page and prevent them from even getting to the first step. I believe we should follow this pattern, unless there's a good reason not to.
This is a good point. I would consider multiple training sources with different language codes a hard stop as I don't believe Serval can handle it. I could go either way on the changes in the PR being a hard or soft stop. Serval will handle training and drafting sources being different language codes (but draft quality will be poor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cool. It sounds like you have more recent information than I do. Carry on!
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.html
line 55 at r1 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
This is a good point. I would consider multiple training sources with different language codes a hard stop as I don't believe Serval can handle it. I could go either way on the changes in the PR being a hard or soft stop. Serval will handle training and drafting sources being different language codes (but draft quality will be poor).
Right. As the PR is currently, it is a hard stop, but it's hard stopping too late in the process. If we're going to hard stop, we need to hard stop at the "landing" page, where the other hard stops are.
Given that the risk/cost of a poor-quality draft is not significant and can be resolved by the user, we should default toward warning but not prohibiting users from choosing settings that look like they are incorrect. To clarify, the trade-off I'm assuming is either
The first bad outcome seems preferrable. |
Thanks for the considerations. I think the best way forward is to warn users on the confirm page when language codes are not equivalent between training and drafting languages, and then that means we do not have warnings on the generation page when their language codes do not match. The user will be able to proceed to generate the draft even if these warnings are visible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense. Can you update the JIRA task (title, description, acceptance tests, etc.), as well as updating this PR, to match this new direction?
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)
8f68f24
to
17d5624
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting an error when starting a new draft.
Reviewable status: 3 of 12 files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)
Changing to be a draft |
17d5624
to
29dc49f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raymond, thanks for your work on this. I really like the improvements you've made to this! I noticed a few things that need your attention.
Reviewed 3 of 9 files at r3, 13 of 13 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 9 at r4 (raw file):
{{ i18n.enumerateList(sourceSideLanguageCodes) }}. Using incorrect language codes, or using source or reference projects that are not all in the same language, may significantly reduce the draft quality. </p>
Please localize the notice messages on this page.
Code quote:
<h3>All source and reference projects should be in the same language</h3>
<p>
Source and reference projects currently have the following language codes:
{{ i18n.enumerateList(sourceSideLanguageCodes) }}. Using incorrect language codes, or using source or reference
projects that are not all in the same language, may significantly reduce the draft quality.
</p>
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 10 at r4 (raw file):
projects that are not all in the same language, may significantly reduce the draft quality. </p> <p>{{ t("how_to_change_language_codes") }}</p>
Until the new UI for draft config is complete, we should have a link to the settings page for admins. Translators should also see a message to contact administrator if language settings need changed.
Code quote:
{{ t("how_to_change_language_codes") }}
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 26 at r4 (raw file):
the source and target projects set to the same language may significantly reduce the draft quality. </p> <p>{{ t("how_to_change_language_codes") }}</p>
See previous comment.
Code quote:
<p>{{ t("how_to_change_language_codes") }}</p>
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 37 at r4 (raw file):
<h3>{{ t("incorrect_language_codes_reduce_quality") }}</h3> <p>Please make sure all the language codes are correct before saving.</p> <p>{{ t("how_to_change_language_codes") }}</p>
nit Do we need to show this if all language codes are equivalent?
Code quote:
<p>{{ t("how_to_change_language_codes") }}</p>
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.spec.ts
line 60 at r4 (raw file):
}); // TODO: test that language codes confirmed emits
nit Were you going to add a test for this?
Code quote:
// TODO: test that language codes confirmed emits
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 123 at r4 (raw file):
"references_heading": "References ({{ referenceLanguage }})", "review_draft_setup": "Review draft setup", "sources_must_be_same_language": "The training source must be the same language as the drafting source. You can update your source texts on the [link:projectSettingsUrl]settings page[/link]",
This could be used instead of "how to change language codes". The info_alert...
could be updated to localize the language-codes-confirmation.html page.
Code quote:
"sources_must_be_same_language": "The training source must be the same language as the drafting source. You can update your source texts on the [link:projectSettingsUrl]settings page[/link]"
@RaymondLuong3 I tested this locally with the sole source set to an English resource, with my project also set to English, and I get this page: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this. I have fixed the issue causing this.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @josephmyers and @kylebuss)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.html
line 55 at r1 (raw file):
Previously, josephmyers wrote…
Right. As the PR is currently, it is a hard stop, but it's hard stopping too late in the process. If we're going to hard stop, we need to hard stop at the "landing" page, where the other hard stops are.
Based on the direction that Nathaniel and I chatted about, we are not moving any language code related warnings into the draft stepper component. I've stripped away the language code warnings from the draft generation page.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 9 at r4 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
Please localize the notice messages on this page.
Thanks. Some of the wording may still be changed, but I think it is safe now to internationalize this component and make adjustments as needed.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 10 at r4 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
Until the new UI for draft config is complete, we should have a link to the settings page for admins. Translators should also see a message to contact administrator if language settings need changed.
Good idea. I've updated it to show a message with a link to the settings page
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 26 at r4 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
See previous comment.
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 37 at r4 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
nit Do we need to show this if all language codes are equivalent?
I have left it as is. This component's messaging reflects what is in the confirm draft sources example and I didn't want to move too far away from it.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.spec.ts
line 60 at r4 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
nit Were you going to add a test for this?
Done
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 123 at r4 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
This could be used instead of "how to change language codes". The
info_alert...
could be updated to localize the language-codes-confirmation.html page.
I have removed this and similar strings from this file
29dc49f
to
641abd3
Compare
@@ -36,6 +36,7 @@ | |||
// this mismatch is left to be solved at another time. | |||
|
|||
export const IGNORE_COOKIE_LOCALE = new InjectionToken<boolean>('IGNORE_COOKIE_LOCALE'); | |||
const LANGUAGE_CODE_REGEX = /(^[a-zA-Z]{2,3})-*/; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 5 days ago
To fix the problem, we need to remove the unused variable LANGUAGE_CODE_REGEX
from the code. This will make the code cleaner and more maintainable. The change should be made in the file src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts
by deleting the line where LANGUAGE_CODE_REGEX
is declared.
-
Copy modified line R39
@@ -38,3 +38,3 @@ | ||
export const IGNORE_COOKIE_LOCALE = new InjectionToken<boolean>('IGNORE_COOKIE_LOCALE'); | ||
const LANGUAGE_CODE_REGEX = /(^[a-zA-Z]{2,3})-*/; | ||
// Removed unused variable LANGUAGE_CODE_REGEX | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 9 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: 16 of 18 files reviewed, 8 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.ts
line 24 at r6 (raw file):
export class LanguageCodesConfirmationComponent { @Output() languageCodesVerified = new EventEmitter<boolean>(false); @Input() set draftSources(value: DraftSourcesAsArrays | undefined) {
It would make this class easier to use if it retrieved this information itself, instead of needing it to be supplied. See ConfirmSourcesComponent
for an example of this.
Good work. One bug I notice is:
It should say:
I think this can be fixed with: if (languageNames.length < 2) {
return [sourceLanguagesCodes[0]];
}
- return sourceLanguagesCodes;
+ return Array.from(new Set(sourceLanguagesCodes));
} |
Sorry; did not mean to close. Was hitting the wrong keys on the keyboard. I just pushed a minor CSS change that I think improves the appearance and layout. It brings it more in line with the design we've been using. |
9a42f94
to
886d063
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also added a storybook for the language codes confirmation for the 3 different states of the language codes confirmation component.
Reviewable status: 9 of 19 files reviewed, 8 unresolved discussions (waiting on @Github-advanced-security[bot], @josephmyers, and @kylebuss)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.ts
line 24 at r6 (raw file):
Previously, josephmyers wrote…
It would make this class easier to use if it retrieved this information itself, instead of needing it to be supplied. See
ConfirmSourcesComponent
for an example of this.
Thanks. I was thinking since we already had that information we could pass it into the component, but I see the advantage of giving the component direct access to the draft sources. Done.
@@ -36,6 +36,7 @@ | |||
// this mismatch is left to be solved at another time. | |||
|
|||
export const IGNORE_COOKIE_LOCALE = new InjectionToken<boolean>('IGNORE_COOKIE_LOCALE'); | |||
const LANGUAGE_CODE_REGEX = /(^[a-zA-Z]{2,3})-*/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 9 files at r5, 2 of 2 files at r7, 7 of 7 files at r8, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Github-advanced-security[bot], @josephmyers, and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.html
line 37 at r4 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I have left it as is. This component's messaging reflects what is in the confirm draft sources example and I didn't want to move too far away from it.
That's fair, I would say it should follow the same admin/non-admin message as above.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 404 at r8 (raw file):
"change_source_projects_on_settings_page": "Source projects can be changed on the [link:projectSettingsUrl]settings page[/link]", "confirm_lang_codes_correct": "All the language codes are correct.", "contact_project_administrator_to_change_settings": "Contact your project administrator to change project settings",
nit Consider: "Contact your project administrator if language settings need to be updated"
Code quote:
"Contact your project administrator to change project settings",
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 407 at r8 (raw file):
"how_to_change_language_codes": "Language codes can be changed in Paratext by going to Project Settings > Project Properties > Language.", "incorrect_language_codes_reduce_quality": "Incorrect language codes will dramatically reduce draft quality.", "i_understand_and_accept": "I understand the problem and want to save these settings anyway",
nit Consider changing the wording to: "I understand the problem and want to proceed anyways."
Code quote:
I understand the problem and want to save these settings anyway
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 408 at r8 (raw file):
"incorrect_language_codes_reduce_quality": "Incorrect language codes will dramatically reduce draft quality.", "i_understand_and_accept": "I understand the problem and want to save these settings anyway", "please_make_sure_codes_correct": "Please make sure all the language codes are correct before saving.",
nit Consider changing to "proceeding."
Code quote:
saving.
I would prefer "continue" over "proceed". Though in the future we may change it back based on where this component is used. |
ede1ac9
to
e106162
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've updated some of the strings that this affects
Reviewable status: 18 of 19 files reviewed, 3 unresolved discussions (waiting on @Github-advanced-security[bot] and @josephmyers)
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 404 at r8 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
nit Consider: "Contact your project administrator if language settings need to be updated"
Done.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 407 at r8 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
nit Consider changing the wording to: "I understand the problem and want to proceed anyways."
Done.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 408 at r8 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
nit Consider changing to "proceeding."
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Github-advanced-security[bot] and @josephmyers)
ea359c5
to
386f574
Compare
@Nateowami I think this is good to go once the chromatic changes are accepted. Could you dismiss Joseph's review, I've addressed what he asked for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @Github-advanced-security[bot] and @josephmyers)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts
line 239 at r7 (raw file):
this.isBackTranslation = translateConfig?.projectType === ProjectType.BackTranslation; this.isSourceProjectSet = translateConfig?.source?.projectRef !== undefined;
This logic is really no longer correct.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 402 at r9 (raw file):
"language_codes_confirmation": { "all_sources_should_be_same_language": "All source and reference projects should be in the same language", "change_source_projects_on_settings_page": "Source projects can be changed on the [link:projectSettingsUrl]settings page[/link]",
`
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmatino.stories.ts
line 1 at r10 (raw file):
import { CommonModule } from '@angular/common';
The file name has a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @Github-advanced-security[bot], @josephmyers, and @Nateowami)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts
line 239 at r7 (raw file):
Previously, Nateowami wrote…
This logic is really no longer correct.
Do you mean that users should be able to generate a draft if they don't explicitly have a source set, but they do have alternate training and drafting source set? If so, we can create a different issue for that.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmatino.stories.ts
line 1 at r10 (raw file):
Previously, Nateowami wrote…
The file name has a typo.
Done.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 402 at r9 (raw file):
Previously, Nateowami wrote…
`
Was there a comment with this?
1dbc3aa
to
044786b
Compare
This PR adds a warning for users when their training and drafting sources have different languages. It is not so simple just to compare language codes because some languages may have multiple valid codes. My approach was to take the Intl.DisplayNames() function and compare the English names if the codes are different. I am open to suggestions for better approaches.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)