Skip to content

Commit

Permalink
[GSoC'24] Improvements to modify translations modal (oppia#20585)
Browse files Browse the repository at this point in the history
* Improve modal CSS

* Tick checkbox on saving a translation

* Update button text for clarity

* Show subsidiary modal only when editable translations exist

* Verify if RTE components of modified translation match when updating translation

* Add missing languages to supported content language list

* Fix frontend tests

* Fix acceptance test

* Fix bugs pertaining to stale and empty translations

* Fix linter issues

* Gate translation changes button behind feature flag

* Fix frontend test
  • Loading branch information
Vir-8 authored Jul 3, 2024
1 parent 94cf044 commit 0b56085
Show file tree
Hide file tree
Showing 20 changed files with 431 additions and 456 deletions.
58 changes: 47 additions & 11 deletions assets/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5139,11 +5139,11 @@ export default {
"decimal_separator": ".",
"ariaLabelInEnglish": "English"
}, {
"code": "ar",
"description": "العربية (Arabic)",
"direction": "rtl",
"decimal_separator": ",",
"ariaLabelInEnglish": "Arabic"
"code": "ak",
"description": "Ákán (Akan)",
"direction": "ltr",
"decimal_separator": ".",
"ariaLabelInEnglish": "Akan"
}, {
"code": "sq",
"description": "shqip (Albanian)",
Expand All @@ -5156,6 +5156,12 @@ export default {
"direction": "ltr",
"decimal_separator": ".",
"ariaLabelInEnglish": "Amharic"
}, {
"code": "ar",
"description": "العربية (Arabic)",
"direction": "rtl",
"decimal_separator": ",",
"ariaLabelInEnglish": "Arabic"
}, {
"code": "az",
"description": "Azeri (Azerbaijani)",
Expand All @@ -5174,6 +5180,12 @@ export default {
"direction": "ltr",
"decimal_separator": ".",
"ariaLabelInEnglish": "Bangla"
}, {
"code": "ms",
"description": "بهاس ملايو (Bahasa Melayu)",
"direction": "ltr",
"decimal_separator": ".",
"ariaLabelInEnglish": "Bahasa Melayu"
}, {
"code": "ca",
"description": "català (Catalan)",
Expand Down Expand Up @@ -5204,12 +5216,24 @@ export default {
"direction": "ltr",
"decimal_separator": ",",
"ariaLabelInEnglish": "Danish"
}, {
"code": "prs",
"description": "دری (Dari)",
"direction": "rtl",
"decimal_separator": ",",
"ariaLabelInEnglish": "Dari"
}, {
"code": "nl",
"description": "Nederlands (Dutch)",
"direction": "ltr",
"decimal_separator": ",",
"ariaLabelInEnglish": "Dutch"
}, {
"code": "ee",
"description": "Eʋegbe (Ewe)",
"direction": "ltr",
"decimal_separator": ",",
"ariaLabelInEnglish": "Ewe"
}, {
"code": "fat",
"description": "Fanti",
Expand Down Expand Up @@ -5252,6 +5276,12 @@ export default {
"direction": "ltr",
"decimal_separator": ",",
"ariaLabelInEnglish": "Greek"
}, {
"code": "gaa",
"description": "Gã (Ga)",
"direction": "ltr",
"decimal_separator": ",",
"ariaLabelInEnglish": "Ga"
}, {
"code": "ha",
"description": "Harshen Hausa (Hausa)",
Expand Down Expand Up @@ -5360,18 +5390,18 @@ export default {
"direction": "ltr",
"decimal_separator": ",",
"ariaLabelInEnglish": "Polish"
}, {
"code": "prs",
"description": "دری (Dari)",
"direction": "rtl",
"decimal_separator": ".",
"ariaLabelInEnglish": "Dari"
}, {
"code": "pt",
"description": "português (Portuguese)",
"direction": "ltr",
"decimal_separator": ",",
"ariaLabelInEnglish": "Portuguese"
}, {
"code": "ps",
"description": "پښتو (Pashto)",
"direction": "rtl",
"decimal_separator": ",",
"ariaLabelInEnglish": "Pashto"
}, {
"code": "ro",
"description": "română (Romanian)",
Expand Down Expand Up @@ -5426,6 +5456,12 @@ export default {
"direction": "ltr",
"decimal_separator": ".",
"ariaLabelInEnglish": "Tamil"
}, {
"code": "te",
"description": "తెలుగు (Telugu)",
"direction": "ltr",
"decimal_separator": ".",
"ariaLabelInEnglish": "Telugu"
}, {
"code": "th",
"description": "ภาษาไทย (Thai)",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ import {
import {MarkTranslationsAsNeedingUpdateModalComponent} from './mark-translations-as-needing-update-modal.component';
import {PlatformFeatureService} from 'services/platform-feature.service';
import {ModifyTranslationsModalComponent} from '../../../pages/exploration-editor-page/modal-templates/exploration-modify-translations-modal.component';
import {EntityTranslationsService} from 'services/entity-translations.services';
import {TranslatedContent} from 'domain/exploration/TranslatedContentObjectFactory';
import {EntityTranslation} from 'domain/translation/EntityTranslationObjectFactory';

class MockActiveModal {
close(): void {
Expand Down Expand Up @@ -67,6 +70,7 @@ describe('Mark Translations As Needing Update Modal Component', () => {
let ngbModal: NgbModal;
let mockPlatformFeatureService = new MockPlatformFeatureService();
let ngbModalRef: MockNgbModalRef = new MockNgbModalRef();
let entityTranslationsService: EntityTranslationsService;

beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
Expand All @@ -93,6 +97,23 @@ describe('Mark Translations As Needing Update Modal Component', () => {

ngbActiveModal = TestBed.inject(NgbActiveModal);
ngbModal = TestBed.inject(NgbModal);
entityTranslationsService = TestBed.inject(EntityTranslationsService);

entityTranslationsService.languageCodeToLatestEntityTranslations = {
hi: EntityTranslation.createFromBackendDict({
entity_id: 'expId',
entity_type: 'exploration',
entity_version: 5,
language_code: 'hi',
translations: {
content1: {
content_value: 'This is text one.',
content_format: 'html',
needs_update: true,
},
},
}),
};
}));

it('should check whether component is initialized', () => {
Expand Down Expand Up @@ -120,12 +141,15 @@ describe('Mark Translations As Needing Update Modal Component', () => {
});

it('should open the ModifyTranslations modal', fakeAsync(() => {
component.contentId = 'content0';
component.contentId = 'content1';
component.contentValue = 'Content value';
ngbModalRef.result = Promise.resolve();
const modalSpy = spyOn(ngbModal, 'open').and.returnValue(
ngbModalRef as NgbModalRef
);
spyOn(component, 'doesContentHaveDisplayableTranslations').and.returnValue(
true
);
spyOn(ngbActiveModal, 'close');

component.openModifyTranslationsModal();
Expand All @@ -135,17 +159,20 @@ describe('Mark Translations As Needing Update Modal Component', () => {
backdrop: 'static',
windowClass: 'oppia-modify-translations-modal',
});
expect(ngbModalRef.componentInstance.contentId).toBe('content0');
expect(ngbModalRef.componentInstance.contentId).toBe('content1');
expect(ngbModalRef.componentInstance.contentValue).toBe('Content value');
expect(ngbActiveModal.close).toHaveBeenCalled();
}));

it('should cancel ModifyTranslations modal', () => {
component.contentId = 'content0';
component.contentId = 'content1';
ngbModalRef.result = Promise.reject();
const modalSpy = spyOn(ngbModal, 'open').and.returnValue(
ngbModalRef as NgbModalRef
);
spyOn(component, 'doesContentHaveDisplayableTranslations').and.returnValue(
true
);
spyOn(ngbActiveModal, 'close');

component.openModifyTranslationsModal();
Expand All @@ -154,10 +181,21 @@ describe('Mark Translations As Needing Update Modal Component', () => {
backdrop: 'static',
windowClass: 'oppia-modify-translations-modal',
});
expect(ngbModalRef.componentInstance.contentId).toBe('content0');
expect(ngbModalRef.componentInstance.contentId).toBe('content1');
expect(ngbActiveModal.close).not.toHaveBeenCalled();
});

it('should dismiss current modal if no editable translations exist', () => {
component.contentId = 'content1';
spyOn(ngbModal, 'open');
spyOn(ngbActiveModal, 'close');

component.openModifyTranslationsModal();

expect(ngbModal.open).not.toHaveBeenCalled();
expect(ngbActiveModal.close).toHaveBeenCalled();
});

it('should call removeTranslations', () => {
const handlerWithSpy = jasmine.createSpy();
component.removeHandler = handlerWithSpy;
Expand All @@ -174,4 +212,21 @@ describe('Mark Translations As Needing Update Modal Component', () => {

expect(dismissSpy).toHaveBeenCalled();
});

it('should determine if content has displayable translations', () => {
component.contentId = 'content1';

expect(component.doesContentHaveDisplayableTranslations()).toBe(false);

entityTranslationsService.languageCodeToLatestEntityTranslations.hi.updateTranslation(
'content1',
TranslatedContent.createFromBackendDict({
content_value: 'This is text one.',
content_format: 'html',
needs_update: false,
})
);

expect(component.doesContentHaveDisplayableTranslations()).toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {NgbModal} from '@ng-bootstrap/ng-bootstrap';
import {ConfirmOrCancelModal} from 'components/common-layout-directives/common-elements/confirm-or-cancel-modal.component';
import {PlatformFeatureService} from 'services/platform-feature.service';
import {ModifyTranslationsModalComponent} from 'pages/exploration-editor-page/modal-templates/exploration-modify-translations-modal.component';
import {EntityTranslationsService} from 'services/entity-translations.services';

@Component({
selector: 'oppia-mark-translations-as-needing-update-modal',
Expand All @@ -39,7 +40,8 @@ export class MarkTranslationsAsNeedingUpdateModalComponent extends ConfirmOrCanc
constructor(
private ngbActiveModal: NgbActiveModal,
private ngbModal: NgbModal,
private platformFeatureService: PlatformFeatureService
private platformFeatureService: PlatformFeatureService,
private entityTranslationsService: EntityTranslationsService
) {
super(ngbActiveModal);
}
Expand All @@ -55,22 +57,41 @@ export class MarkTranslationsAsNeedingUpdateModalComponent extends ConfirmOrCanc
}

openModifyTranslationsModal(): void {
const modalRef = this.ngbModal.open(ModifyTranslationsModalComponent, {
backdrop: 'static',
windowClass: 'oppia-modify-translations-modal',
});
modalRef.componentInstance.contentId = this.contentId;
modalRef.componentInstance.contentValue = this.contentValue;
modalRef.result.then(
result => {
this.ngbActiveModal.close();
},
() => {
// Note to developers:
// This callback is triggered when the Cancel button is clicked.
// No further action is needed.
if (this.doesContentHaveDisplayableTranslations()) {
const modalRef = this.ngbModal.open(ModifyTranslationsModalComponent, {
backdrop: 'static',
windowClass: 'oppia-modify-translations-modal',
});
modalRef.componentInstance.contentId = this.contentId;
modalRef.componentInstance.contentValue = this.contentValue;
modalRef.result.then(
result => {
this.ngbActiveModal.close();
},
() => {
// Note to developers:
// This callback is triggered when the Cancel button is clicked.
// No further action is needed.
}
);
} else {
this.ngbActiveModal.close();
}
}

doesContentHaveDisplayableTranslations(): boolean {
// Check if at least one translation is editable by lesson creator.
for (let language in this.entityTranslationsService
.languageCodeToLatestEntityTranslations) {
let translationContent =
this.entityTranslationsService.languageCodeToLatestEntityTranslations[
language
].getWrittenTranslation(this.contentId);
if (translationContent && !translationContent.needsUpdate) {
return true;
}
);
}
return false;
}

removeTranslations(): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -906,9 +906,18 @@ describe('Translation Modal Component', () => {

it('should close modal and return the new translation when updating translated text', () => {
spyOn(activeModal, 'close');
spyOn(component, 'translatedTextCanBeSubmitted').and.returnValue(true);
component.activeWrittenTranslation = 'Test translation';
component.updateTranslatedText();

expect(activeModal.close).toHaveBeenCalledWith('Test translation');
});

it('should not close modal if new translated text cannot be submitted', () => {
spyOn(activeModal, 'close');
component.activeWrittenTranslation = 'Test translation';
component.updateTranslatedText();

expect(activeModal.close).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ export class TranslationModalComponent {
);
}

suggestTranslatedText(): void {
translatedTextCanBeSubmitted(): boolean {
if (!this.isSetOfStringDataFormat()) {
const domParser = new DOMParser();
const originalElements = domParser.parseFromString(
Expand Down Expand Up @@ -604,13 +604,20 @@ export class TranslationModalComponent {
this.uploadingTranslation ||
this.loadingData
) {
return;
return false;
}

if (this.hadCopyParagraphError) {
this.hadCopyParagraphError = false;
}
}
return true;
}

suggestTranslatedText(): void {
if (!this.translatedTextCanBeSubmitted()) {
return;
}

if (!this.uploadingTranslation && !this.loadingData) {
this.siteAnalyticsService.registerContributorDashboardSubmitSuggestionEvent(
Expand Down Expand Up @@ -650,6 +657,9 @@ export class TranslationModalComponent {
}
}
updateTranslatedText(): void {
if (!this.translatedTextCanBeSubmitted()) {
return;
}
this.activeModal.close(this.activeWrittenTranslation);
}
}
Expand Down
Loading

0 comments on commit 0b56085

Please sign in to comment.