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

[GSoC'24] M1.3 - Modify translations frontend modal #20439

Merged
merged 36 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
30cb556
Create frontend modal with list of translations populated
Vir-8 Jun 7, 2024
f9ed166
Display language names appropriately
Vir-8 Jun 7, 2024
a0eb666
Stylize scrollbar and fix displaying rich text elements
Vir-8 Jun 7, 2024
80f2b9c
Fix button styles
Vir-8 Jun 8, 2024
ed19d3f
Handle populating draft translation changes appropriately
Vir-8 Jun 8, 2024
020b0c9
Add comments for clarity
Vir-8 Jun 8, 2024
4e32851
Handle cases where translations have been removed and no translations…
Vir-8 Jun 8, 2024
44e07a7
Handle logic for removing published as well as draft translations app…
Vir-8 Jun 8, 2024
e59dd79
Merge remote-tracking branch 'upstream/develop' into modify-translati…
Vir-8 Jun 9, 2024
ff26a22
Add frontend tests
Vir-8 Jun 11, 2024
597ebb3
Write acceptance tests
Vir-8 Jun 11, 2024
2e9e668
Fix lint errors and convert backend dicts appropriately
Vir-8 Jun 11, 2024
8dedc2a
Merge remote-tracking branch 'upstream/develop' into modify-translati…
Vir-8 Jun 11, 2024
92ba7b2
Fix merge conflict
Vir-8 Jun 11, 2024
837e723
Fix lint issue
Vir-8 Jun 11, 2024
793ef8a
Fix typescript issues
Vir-8 Jun 11, 2024
d92ea35
Fix merge issue
Vir-8 Jun 11, 2024
17e57ca
Merge remote-tracking branch 'upstream/develop' into modify-translati…
Vir-8 Jun 11, 2024
dbb7fca
Fix frontend test failure
Vir-8 Jun 12, 2024
c8ac939
Fix acceptance testing, add feature flag utility
Vir-8 Jun 13, 2024
1361475
Merge remote-tracking branch 'upstream/develop' into modify-translati…
Vir-8 Jun 13, 2024
ffbbb33
Fix merge issues
Vir-8 Jun 13, 2024
bfa068b
Fix lint issues
Vir-8 Jun 13, 2024
17e050c
Fix typo
Vir-8 Jun 14, 2024
690499b
Make release coordinator page responsive and fix acceptance testing
Vir-8 Jun 14, 2024
2d5bca3
Merge remote-tracking branch 'upstream/develop' into modify-translati…
Vir-8 Jun 14, 2024
f835f94
Fix lint issues
Vir-8 Jun 14, 2024
cf85be6
Fix e2e test failing
Vir-8 Jun 14, 2024
df70958
Attempt to fix acceptance test
Vir-8 Jun 14, 2024
e9fd519
Remove acceptance test from CI
Vir-8 Jun 14, 2024
5f1fcbc
Add todo comment for acceptance testing
Vir-8 Jun 14, 2024
9401aa2
Address comments
Vir-8 Jun 15, 2024
64611c8
Address comments
Vir-8 Jun 16, 2024
196b344
Fix lint issue
Vir-8 Jun 16, 2024
4553490
Merge remote-tracking branch 'upstream/develop' into modify-translati…
Vir-8 Jun 18, 2024
13d3fc4
Merge remote-tracking branch 'upstream/develop' into modify-translati…
Vir-8 Jun 19, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,8 @@ jobs:
strategy:
matrix:
os: [ubuntu-22.04]
# TODO(#20467): Add exploration-editor/modify-translations-through-modal spec file
# to suites once the exploration_editor_can_modify_translations feature is in prod stage.
suite:
- blog-admin/assign-roles-to-users-and-change-tag-properties
- blog-editor/create-and-delete-draft-blog-post
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,15 @@ <h3>Update Translations</h3>
</p>
</li>
<li>
<p>
<p *ngIf="!modifyTranslationsFeatureFlagIsEnabled">
<strong>Mark translations as stale:</strong>
Mark the existing translations for the given content as stale. Select this if your changes will require the translations
to be updated, but the previous translations can still be used.
</p>
<p *ngIf="modifyTranslationsFeatureFlagIsEnabled">
<strong>Modify existing translations:</strong>
Select this if your changes mean that existing translations will need to be modified slightly.
</p>
</li>
<li>
<p>
Expand All @@ -41,6 +45,7 @@ <h3>Update Translations</h3>

<div class="modal-footer">
<button class="btn btn-secondary" (click)="removeTranslations()">Remove existing translations</button>
<button class="btn btn-secondary" (click)="markNeedsUpdate()">Mark translations as stale</button>
<button *ngIf="!modifyTranslationsFeatureFlagIsEnabled"class="btn btn-secondary" (click)="markNeedsUpdate()">Mark translations as stale</button>
<button *ngIf="modifyTranslationsFeatureFlagIsEnabled" class="btn btn-secondary e2e-test-modify-translations-button" (click)="openModifyTranslationsModal()">Modify existing translations</button>
<button class="btn btn-secondary e2e-test-leave-translations-as-is" (click)="cancel()">Leave as is</button>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,14 @@
import {NO_ERRORS_SCHEMA} from '@angular/core';
import {HttpClientTestingModule} from '@angular/common/http/testing';
import {ComponentFixture, waitForAsync, TestBed} from '@angular/core/testing';
import {NgbActiveModal} from '@ng-bootstrap/ng-bootstrap';
import {
NgbActiveModal,
NgbModal,
NgbModalRef,
} from '@ng-bootstrap/ng-bootstrap';
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';

class MockActiveModal {
close(): void {
Expand All @@ -32,10 +38,27 @@ class MockActiveModal {
}
}

class MockPlatformFeatureService {
status = {
ExplorationEditorCanModifyTranslations: {
isEnabled: false,
},
};
}

export class MockNgbModalRef {
componentInstance = {
contentId: null,
};
}

describe('Mark Translations As Needing Update Modal Component', () => {
let component: MarkTranslationsAsNeedingUpdateModalComponent;
let fixture: ComponentFixture<MarkTranslationsAsNeedingUpdateModalComponent>;
let ngbActiveModal: NgbActiveModal;
let ngbModal: NgbModal;
let mockPlatformFeatureService = new MockPlatformFeatureService();
let ngbModalRef: MockNgbModalRef = new MockNgbModalRef();

beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
Expand All @@ -46,6 +69,10 @@ describe('Mark Translations As Needing Update Modal Component', () => {
provide: NgbActiveModal,
useClass: MockActiveModal,
},
{
provide: PlatformFeatureService,
useValue: mockPlatformFeatureService,
},
],
schemas: [NO_ERRORS_SCHEMA],
}).compileComponents();
Expand All @@ -57,12 +84,22 @@ describe('Mark Translations As Needing Update Modal Component', () => {
fixture.detectChanges();

ngbActiveModal = TestBed.inject(NgbActiveModal);
ngbModal = TestBed.inject(NgbModal);
}));

it('should check whether component is initialized', () => {
expect(component).toBeDefined();
});

it('should check feature flag when initialized', () => {
mockPlatformFeatureService.status.ExplorationEditorCanModifyTranslations.isEnabled =
true;

component.ngOnInit();

expect(component.modifyTranslationsFeatureFlagIsEnabled).toBeTrue();
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a pre-check for this? i.e. the value is false before ngOnInit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

});

it('should call markNeedingUpdateHandler', () => {
const handlerWithSpy = jasmine.createSpy();
component.markNeedsUpdateHandler = handlerWithSpy;
Expand All @@ -73,6 +110,21 @@ describe('Mark Translations As Needing Update Modal Component', () => {
expect(handlerWithSpy).toHaveBeenCalledOnceWith('contentId_1');
});

it('should open the ModifyTranslations modal', () => {
component.contentId = 'content0';
const modalSpy = spyOn(ngbModal, 'open').and.returnValue(
ngbModalRef as NgbModalRef
);

component.openModifyTranslationsModal();

expect(modalSpy).toHaveBeenCalledWith(ModifyTranslationsModalComponent, {
backdrop: 'static',
windowClass: 'oppia-modify-translations-modal',
});
expect(ngbModalRef.componentInstance.contentId).toBe('content0');
});

it('should call removeTranslations', () => {
const handlerWithSpy = jasmine.createSpy();
component.removeHandler = handlerWithSpy;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@

import {Component, Input} from '@angular/core';
import {NgbActiveModal} from '@ng-bootstrap/ng-bootstrap';
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';

@Component({
selector: 'oppia-mark-translations-as-needing-update-modal',
Expand All @@ -30,15 +33,34 @@ export class MarkTranslationsAsNeedingUpdateModalComponent extends ConfirmOrCanc
@Input() markNeedsUpdateHandler!: (contentId: string) => void;
@Input() removeHandler!: (contentId: string) => void;

constructor(private ngbActiveModal: NgbActiveModal) {
modifyTranslationsFeatureFlagIsEnabled: boolean = false;

constructor(
private ngbActiveModal: NgbActiveModal,
private ngbModal: NgbModal,
private platformFeatureService: PlatformFeatureService
) {
super(ngbActiveModal);
}

ngOnInit(): void {
this.modifyTranslationsFeatureFlagIsEnabled =
this.platformFeatureService.status.ExplorationEditorCanModifyTranslations.isEnabled;
Comment on lines +47 to +48
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe its needed here and it is pretty standard to have it this way in the codebase, would be needed if the line to the right side of = was further split up into multiple lines

Copy link
Member

Choose a reason for hiding this comment

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

I think it may have been missed in other places. Let's stick to what is in the style guide.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to ignore wrapping here in case adding parenthesis ends up causing L48 to hit the line character limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevintab95 I tried adding the brackets but they got removed automatically by Prettify itself, I assume it is the standard for such lines...?

Copy link
Member

Choose a reason for hiding this comment

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

I tried a few variations on my machine. From what I can tell, the pre-commit linter does not care even if the line is too long:
Screenshot from 2024-06-16 11-42-48

I was able to successfully commit these lines and no modifications were made by prettier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I tried this variation which seems to revert back
image
image

Copy link
Member

Choose a reason for hiding this comment

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

@nikitaevg do you know why this is happening? i.e. prettier seems to work differently on our machines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, no idea TBH. You sure you did save the file? The commit stats say you added 1 line and modified 1 line, but the screenshot suggests much more changes

}

markNeedsUpdate(): void {
this.markNeedsUpdateHandler(this.contentId);
this.ngbActiveModal.close();
}

openModifyTranslationsModal(): void {
const modalRef = this.ngbModal.open(ModifyTranslationsModalComponent, {
backdrop: 'static',
windowClass: 'oppia-modify-translations-modal',
});
modalRef.componentInstance.contentId = this.contentId;
}

removeTranslations(): void {
this.removeHandler(this.contentId);
this.ngbActiveModal.close();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

<span *ngIf="!getCodingMode() && !getRows()">
<input type="text"
class="form-control display-inline"
class="form-control display-inline e2e-test-text-input"
[(ngModel)]="localValue"
(input)="updateLocalValue($event)"
[disabled]="disabled"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<div class="oppia-readonly-rule-tile"
[ngClass]="{'oppia-editable-section': isEditable}">
<div class="oppia-rule-preview oppia-transition-200">
<div class="oppia-click-to-start-editing"
<div class="oppia-click-to-start-editing e2e-test-edit-solution-explanation"
*ngIf="isEditable" (click)="openExplanationEditor()">
<i class="material-icons oppia-editor-edit-icon float-right"
(keydown.enter)="openExplanationEditor()"
Expand Down Expand Up @@ -45,7 +45,7 @@
Cancel
</button>
<button type="button"
class="btn btn-success"
class="btn btn-success e2e-test-save-solution-explanation-edit"
[disabled]="!stateSolutionService.displayed.explanation.html || isSolutionExplanationLengthExceeded()"
(click)="saveThisExplanation()">
Save
Expand Down
11 changes: 11 additions & 0 deletions core/templates/css/oppia.css
Original file line number Diff line number Diff line change
Expand Up @@ -4066,6 +4066,17 @@ div.oppia-issues-learner-action {
}
}

.oppia-modify-translations-modal {
align-content: center;
}

@media (max-width: 400px) {
/* Prevents overlapping with the mobile navbar elements. */
oppia-mark-translations-as-needing-update-modal {
margin-bottom: 25px;
}
}

.oppia-typeahead-form .dropdown-item.active {
color: #212529;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ export interface EntityTranslationBackendDict {
translations: TranslationMappingDict;
}

export interface languageCodeToEntityTranslationBackendDict {
[languageCode: string]: EntityTranslationBackendDict;
}

interface TranslationMapping {
[contentId: string]: TranslatedContent;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<ul class="dropdown-menu oppia-exploration-editor-tabs-dropdown" aria-labelledby="dropdownMenuButton">
<li class="dropdown-item oppia-exploration-editor-tabs-dropdown-item" [ngClass]="{'uib-dropdown': countWarnings()}">
<a href="#" ngbTooltip="Editor" placement="{{countWarnings() ? 'left' : 'bottom'}}" (click)="selectMainTab()"
class="oppia-exploration-editor-tabs-dropdown-element">
class="oppia-exploration-editor-tabs-dropdown-element e2e-test-mobile-main-tab">
<i class="fa fa-pen oppia-mobile-exploration-editor-tabs-icon"></i>
<span class="oppia-exploration-editor-tabs-dropdown-inner">Editor</span>
</a>
Expand All @@ -23,7 +23,7 @@
</span>
</div>
</li>
<li class="dropdown-item oppia-exploration-editor-tabs-dropdown-item" *ngIf="isUserLoggedIn()" [ngClass]="{'active': getActiveTabName() === 'translation'}">
<li class="dropdown-item oppia-exploration-editor-tabs-dropdown-item e2e-test-mobile-translation-tab" *ngIf="isUserLoggedIn()" [ngClass]="{'active': getActiveTabName() === 'translation'}">
<a href="#" ngbTooltip="Tmaterial-iconsmaterial-iconsranslations and Voiceovers" placement="bottom" (click)="selectTranslationTab()"
class="oppia-exploration-editor-tabs-dropdown-element">
<i class="fa fa-microphone oppia-mobile-exploration-editor-tabs-icon"></i>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ import {HttpClientTestingModule} from '@angular/common/http/testing';
import {ExplorationPermissions} from 'domain/exploration/exploration-permissions.model';
import {WindowRef} from 'services/contextual/window-ref.service';
import {ExplorationPermissionsBackendApiService} from 'domain/exploration/exploration-permissions-backend-api.service';
import {EntityTranslationsService} from 'services/entity-translations.services';
import {EntityTranslation} from 'domain/translation/EntityTranslationObjectFactory';

class MockNgbModalRef {
componentInstance = {};
Expand Down Expand Up @@ -116,6 +118,7 @@ describe('Exploration editor page component', () => {
let registerDeclineTutorialModalEventSpy;
let focusManagerService: FocusManagerService;
let explorationPermissionsBackendApiService: ExplorationPermissionsBackendApiService;
let entityTranslationsService: EntityTranslationsService;
let ngbModal: NgbModal;
let refreshGraphEmitter = new EventEmitter<void>();
let mockRefreshTranslationTabEventEmitter = new EventEmitter<void>();
Expand Down Expand Up @@ -353,6 +356,7 @@ describe('Exploration editor page component', () => {
explorationPermissionsBackendApiService = TestBed.inject(
ExplorationPermissionsBackendApiService
);
entityTranslationsService = TestBed.inject(EntityTranslationsService);

isLocationSetToNonStateEditorTabSpy = spyOn(
rs,
Expand Down Expand Up @@ -822,7 +826,32 @@ describe('Exploration editor page component', () => {
mockInitExplorationPageEmitter
);
explorationData.is_version_of_draft_valid = false;
explorationData.draft_changes = ['data1', 'data2'];
explorationData.draft_changes = [
{
cmd: 'edit_translation',
language_code: 'fr',
content_id: 'content0',
translation: {
content_value: '<p>test content one</p>',
content_format: 'html',
needs_update: false,
},
},
{
cmd: 'edit_translation',
language_code: 'fr',
content_id: 'content4',
translation: {
content_value: '<p>test content four</p>',
content_format: 'html',
needs_update: false,
},
},
{
cmd: 'remove_translations',
content_id: 'content0',
},
];

component.ngOnInit();
});
Expand Down Expand Up @@ -901,6 +930,43 @@ describe('Exploration editor page component', () => {
discardPeriodicTasks();
}));

it('should update entity translations dict with draft changes', fakeAsync(() => {
spyOn(EntityTranslation, 'createFromBackendDict').and.callThrough();
let entityTranslation = EntityTranslation.createFromBackendDict({
entity_id: explorationId,
entity_type: 'exploration',
entity_version: explorationData.version,
language_code: 'fr',
translations: {
content4: {
content_value: '<p>test content four</p>',
content_format: 'html',
needs_update: false,
},
},
});

mockInitExplorationPageEmitter.emit();
tick();

expect(EntityTranslation.createFromBackendDict).toHaveBeenCalledWith({
entity_id: explorationId,
entity_type: 'exploration',
entity_version: explorationData.version,
language_code: 'fr',
translations: {},
});

expect(
entityTranslationsService.languageCodeToEntityTranslations
Copy link
Member

Choose a reason for hiding this comment

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

Please add a pre-check for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

).toEqual({
fr: entityTranslation,
});

flush();
discardPeriodicTasks();
}));

it(
'should start editor tutorial when closing welcome exploration' +
' modal',
Expand Down
Loading
Loading