code review changes

sillsdev · May 25, 2024 · e5e8d11 · e5e8d11
1 parent 3e76798
commit e5e8d11
Show file tree

Hide file tree

Showing 19 changed files with 258 additions and 183 deletions.
diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.spec.ts
@@ -41,60 +41,60 @@ describe('PermissionsService', () => {
 
   it('allows commenters to access Translate', fakeAsync(() => {
     const env = new TestEnvironment();
-    when(mockedUserService.currentUserId).thenReturn('commenter');
+    when(mockedUserService.currentUserId).thenReturn(SFProjectRole.Commenter);
 
     expect(env.service.canAccessTranslate(env.projectDoc)).toBe(true);
-    expect(env.service.canAccessTranslate(env.projectDoc, 'commenter')).toBe(true);
+    expect(env.service.canAccessTranslate(env.projectDoc, SFProjectRole.Commenter)).toBe(true);
   }));
 
   it('allows checkers to access Community Checking if enabled', fakeAsync(() => {
     const env = new TestEnvironment();
-    when(mockedUserService.currentUserId).thenReturn('checker');
+    when(mockedUserService.currentUserId).thenReturn(SFProjectRole.CommunityChecker);
 
     expect(env.service.canAccessCommunityChecking(env.projectDoc)).toBe(true);
-    expect(env.service.canAccessCommunityChecking(env.projectDoc, 'checker')).toBe(true);
+    expect(env.service.canAccessCommunityChecking(env.projectDoc, SFProjectRole.CommunityChecker)).toBe(true);
   }));
 
   it('allows admins to access both', fakeAsync(() => {
     const env = new TestEnvironment();
-    when(mockedUserService.currentUserId).thenReturn('projectAdmin');
+    when(mockedUserService.currentUserId).thenReturn(SFProjectRole.ParatextAdministrator);
 
     expect(env.service.canAccessTranslate(env.projectDoc)).toBe(true);
-    expect(env.service.canAccessTranslate(env.projectDoc, 'projectAdmin')).toBe(true);
+    expect(env.service.canAccessTranslate(env.projectDoc, SFProjectRole.ParatextAdministrator)).toBe(true);
     expect(env.service.canAccessCommunityChecking(env.projectDoc)).toBe(true);
-    expect(env.service.canAccessCommunityChecking(env.projectDoc, 'projectAdmin')).toBe(true);
+    expect(env.service.canAccessCommunityChecking(env.projectDoc, SFProjectRole.ParatextAdministrator)).toBe(true);
   }));
 
-  it('doesnt allow checkers to access Translate', fakeAsync(() => {
+  it('does not allow checkers to access Translate', fakeAsync(() => {
     const env = new TestEnvironment();
-    when(mockedUserService.currentUserId).thenReturn('checker');
+    when(mockedUserService.currentUserId).thenReturn(SFProjectRole.CommunityChecker);
 
     expect(env.service.canAccessTranslate(env.projectDoc)).toBe(false);
-    expect(env.service.canAccessTranslate(env.projectDoc, 'checker')).toBe(false);
+    expect(env.service.canAccessTranslate(env.projectDoc, SFProjectRole.CommunityChecker)).toBe(false);
   }));
 
-  it('doesnt allow commenters to access Community Checking', fakeAsync(() => {
+  it('does not allow commenters to access Community Checking', fakeAsync(() => {
     const env = new TestEnvironment();
-    when(mockedUserService.currentUserId).thenReturn('commenter');
+    when(mockedUserService.currentUserId).thenReturn(SFProjectRole.Commenter);
 
     expect(env.service.canAccessCommunityChecking(env.projectDoc)).toBe(false);
-    expect(env.service.canAccessCommunityChecking(env.projectDoc, 'commenter')).toBe(false);
+    expect(env.service.canAccessCommunityChecking(env.projectDoc, SFProjectRole.Commenter)).toBe(false);
   }));
 
-  it('doesnt allow checkers to access Community Checking if not enabled', fakeAsync(() => {
+  it('does not allow checkers to access Community Checking if not enabled', fakeAsync(() => {
     const env = new TestEnvironment(false);
-    when(mockedUserService.currentUserId).thenReturn('checker');
+    when(mockedUserService.currentUserId).thenReturn(SFProjectRole.CommunityChecker);
 
     expect(env.service.canAccessCommunityChecking(env.projectDoc)).toBe(false);
-    expect(env.service.canAccessCommunityChecking(env.projectDoc, 'checker')).toBe(false);
+    expect(env.service.canAccessCommunityChecking(env.projectDoc, SFProjectRole.CommunityChecker)).toBe(false);
   }));
 
-  it('doesnt allow admins to access Community Checking if not enabled', fakeAsync(() => {
+  it('does not allow admins to access Community Checking if not enabled', fakeAsync(() => {
     const env = new TestEnvironment(false);
-    when(mockedUserService.currentUserId).thenReturn('projectAdmin');
+    when(mockedUserService.currentUserId).thenReturn(SFProjectRole.ParatextAdministrator);
 
     expect(env.service.canAccessCommunityChecking(env.projectDoc)).toBe(false);
-    expect(env.service.canAccessCommunityChecking(env.projectDoc, 'projectAdmin')).toBe(false);
+    expect(env.service.canAccessCommunityChecking(env.projectDoc, SFProjectRole.ParatextAdministrator)).toBe(false);
   }));
 
   it('allows access to text if user is on project and has permission', fakeAsync(async () => {
@@ -105,7 +105,7 @@ describe('PermissionsService', () => {
     expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(true);
   }));
 
-  it('doesnt allow access to text if user is not on project', fakeAsync(async () => {
+  it('does not allow access to text if user is not on project', fakeAsync(async () => {
     const env = new TestEnvironment();
     env.setCurrentUser('other');
 
@@ -114,14 +114,43 @@ describe('PermissionsService', () => {
     expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(false);
   }));
 
-  it('doesnt allow access to text if user has no access', fakeAsync(async () => {
+  it('does not allow access to text if user has no access', fakeAsync(async () => {
     const env = new TestEnvironment();
     env.setupProjectData(TextInfoPermission.None);
 
     const textDoc: Partial<TextDocId> = { projectId: 'project01', bookNum: 41, chapterNum: 1 };
 
     expect(await env.service.canAccessText(cloneDeep(textDoc) as TextDocId)).toBe(false);
   }));
+
+  describe('canSync', () => {
+    it('returns false when projectDoc.data is undefined', fakeAsync(() => {
+      const env = new TestEnvironment();
+      when(mockedProjectDoc.data).thenReturn(undefined);
+      expect(env.service.canSync(env.projectDoc, SFProjectRole.ParatextAdministrator)).toBe(false);
+    }));
+
+    it('allows users with PT admin role to sync', () => {
+      const env = new TestEnvironment();
+      expect(env.service.canSync(env.projectDoc, SFProjectRole.ParatextAdministrator)).toBe(true);
+    });
+
+    it('allows users with PT translator role to sync', () => {
+      const env = new TestEnvironment();
+      expect(env.service.canSync(env.projectDoc, SFProjectRole.ParatextTranslator)).toBe(true);
+    });
+
+    it('disallows non- PT admin/translator roles to sync', () => {
+      const env = new TestEnvironment();
+      Object.values(SFProjectRole).forEach(role => {
+        if (role === SFProjectRole.ParatextAdministrator || role === SFProjectRole.ParatextTranslator) {
+          return;
+        }
+
+        expect(env.service.canSync(env.projectDoc, role)).toBe(false);
+      });
+    });
+  });
 });
 class TestEnvironment {
   readonly service: PermissionsService;
@@ -130,12 +159,10 @@ class TestEnvironment {
 
   constructor(checkingEnabled = true) {
     this.service = TestBed.inject(PermissionsService);
+    let userRoles = Object.values(SFProjectRole).reduce((roles, role) => ({ ...roles, [role]: role }), {});
+
     const data = createTestProjectProfile({
-      userRoles: {
-        projectAdmin: SFProjectRole.ParatextAdministrator,
-        checker: SFProjectRole.CommunityChecker,
-        commenter: SFProjectRole.Commenter
-      },
+      userRoles,
       checkingConfig: {
         checkingEnabled: checkingEnabled
       }

diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.ts
@@ -59,4 +59,13 @@ export class PermissionsService {
 
     return false;
   }
+
+  canSync(projectDoc: SFProjectProfileDoc, userId: string): boolean {
+    if (projectDoc.data == null) {
+      return false;
+    }
+
+    const role: string = projectDoc.data.userRoles[userId];
+    return role === SFProjectRole.ParatextAdministrator || role === SFProjectRole.ParatextTranslator;
+  }
 }
diff --git a/...orge.Scripture/ClientApp/src/app/shared/sf-tab-group/base-services/tab-factory.service.ts b/...orge.Scripture/ClientApp/src/app/shared/sf-tab-group/base-services/tab-factory.service.ts
@@ -1,3 +1,3 @@
 export abstract class TabFactoryService<TType, T> {
-  abstract createTab(tabType: TType, tabOptions?: Partial<T>): T;
+  abstract createTab(tabType: TType, tabOptions?: Partial<T>): Promise<T>;
 }
diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-group.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-group.component.spec.ts
@@ -69,7 +69,7 @@ describe('TabGroupComponent', () => {
     const tabFactory = TestBed.inject(TabFactoryService);
     const tabStateService = TestBed.inject(TabStateService);
 
-    spyOn(tabFactory, 'createTab').and.returnValue(tab);
+    spyOn(tabFactory, 'createTab').and.returnValue(Promise.resolve(tab));
     spyOn(tabStateService, 'addTab');
 
     component.addTab(newTabType);

diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-group.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-group.component.ts
@@ -63,6 +63,7 @@ export class TabGroupComponent implements OnChanges {
   }
 
   onTabAddRequest(newTabType: string): void {
+    // Some tabs types may need further processing before they can be added (e.g. 'project-resource')
     this.tabAddRequestService
       .handleTabAddRequest(newTabType)
       .pipe(take(1))
@@ -71,8 +72,8 @@ export class TabGroupComponent implements OnChanges {
       });
   }
 
-  addTab(newTabType: string, tabOptions: any = {}): void {
-    const tab = this.tabFactory.createTab(newTabType, tabOptions);
+  async addTab(newTabType: string, tabOptions: any = {}): Promise<void> {
+    const tab = await this.tabFactory.createTab(newTabType, tabOptions);
     this.tabState.addTab(this.groupId, tab);
   }
 

diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-group.stories.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/shared/sf-tab-group/tab-group.stories.ts
@@ -83,7 +83,7 @@ export default {
         {
           provide: TabFactoryService,
           useValue: {
-            createTab(tabType: string): TabInfo<string> {
+            createTab(tabType: string): Promise<TabInfo<string>> {
               let tab: TabInfo<string>;
 
               switch (tabType) {
@@ -114,7 +114,7 @@ export default {
                   break;
               }
 
-              return tab;
+              return Promise.resolve(tab);
             }
           }
         }

diff --git a/...ripture/ClientApp/src/app/translate/editor/editor-resource/editor-resource.component.scss b/...ripture/ClientApp/src/app/translate/editor/editor-resource/editor-resource.component.scss
diff --git a/...ture/ClientApp/src/app/translate/editor/editor-resource/editor-resource.component.spec.ts b/...ture/ClientApp/src/app/translate/editor/editor-resource/editor-resource.component.spec.ts
@@ -39,7 +39,7 @@ describe('EditorResourceComponent', () => {
     component.projectId = undefined;
     component.bookNum = 1;
     component.chapter = 1;
-    component.initProjectDetails();
+    component['initProjectDetails']();
 
     component.resourceText.editorCreated.next();
     verify(mockSFProjectService.getProfile(anything())).never();
@@ -61,7 +61,7 @@ describe('EditorResourceComponent', () => {
     component.bookNum = 1;
     component.chapter = 1;
     when(mockSFProjectService.getProfile(projectId)).thenReturn(Promise.resolve(projectDoc));
-    component.initProjectDetails();
+    component['initProjectDetails']();
     component.resourceText.editorCreated.next();
     tick();
     verify(mockSFProjectService.getProfile(projectId)).once();

diff --git a/...Scripture/ClientApp/src/app/translate/editor/editor-resource/editor-resource.component.ts b/...Scripture/ClientApp/src/app/translate/editor/editor-resource/editor-resource.component.ts
@@ -9,8 +9,7 @@ import { formatFontSizeToRems } from '../../../shared/utils';
 
 @Component({
   selector: 'app-editor-resource',
-  templateUrl: './editor-resource.component.html',
-  styleUrls: ['./editor-resource.component.scss']
+  templateUrl: './editor-resource.component.html'
 })
 export class EditorResourceComponent implements AfterViewInit, OnChanges {
   @Input() projectId?: string;
@@ -39,7 +38,7 @@ export class EditorResourceComponent implements AfterViewInit, OnChanges {
     this.initProjectDetails();
   }
 
-  initProjectDetails(): void {
+  private initProjectDetails(): void {
     combineLatest([this.resourceText.editorCreated, this.inputChanged$.pipe(startWith(undefined))])
       .pipe(
         takeUntilDestroyed(this.destroyRef),

diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts
@@ -70,7 +70,6 @@ import {
 import {
   debounceTime,
   delayWhen,
-  distinctUntilKeyChanged,
   filter,
   first,
   map,
@@ -634,7 +633,7 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
 
   ngOnInit(): void {
     this.activatedProject.projectDoc$
-      .pipe(takeUntilDestroyed(this.destroyRef), filterNullish(), distinctUntilKeyChanged('id'))
+      .pipe(takeUntilDestroyed(this.destroyRef), filterNullish())
       .subscribe(doc => this.initEditorTabs(doc));
   }
 
@@ -1182,55 +1181,55 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
           return Promise.all(
             persistedTabs.map(async tabData => ({
               ...tabData,
-              projectDoc: tabData.projectId ? await this.projectService.getProfile(tabData.projectId) : undefined
+              projectDoc:
+                tabData.projectId != null ? await this.projectService.getProfile(tabData.projectId) : undefined
             }))
           );
         }),
-        tap((persistedTabs: (EditorTabPersistData & { projectDoc?: SFProjectProfileDoc })[]) => {
-          const sourceTabGroup = new TabGroup<EditorTabGroupType, EditorTabInfo>('source');
-          const targetTabGroup = new TabGroup<EditorTabGroupType, EditorTabInfo>('target');
-          const projectSource: TranslateSource | undefined = projectDoc.data?.translateConfig.source;
-
-          if (projectSource != null) {
-            sourceTabGroup.addTab(
-              this.editorTabFactory.createTab('project-source', {
-                projectId: projectSource.projectRef,
-                headerText: projectSource.shortName
-              })
-            );
-          }
-
-          targetTabGroup.addTab(
-            this.editorTabFactory.createTab('project-target', {
-              projectId: projectDoc.id,
-              headerText: projectDoc.data?.shortName
+        take(1)
+      )
+      .subscribe(async (persistedTabs: (EditorTabPersistData & { projectDoc?: SFProjectProfileDoc })[]) => {
+        const sourceTabGroup = new TabGroup<EditorTabGroupType, EditorTabInfo>('source');
+        const targetTabGroup = new TabGroup<EditorTabGroupType, EditorTabInfo>('target');
+        const projectSource: TranslateSource | undefined = projectDoc.data?.translateConfig.source;
+
+        if (projectSource != null) {
+          sourceTabGroup.addTab(
+            await this.editorTabFactory.createTab('project-source', {
+              projectId: projectSource.projectRef,
+              headerText: projectSource.shortName
             })
           );
+        }
 
-          persistedTabs.forEach(tabData => {
-            const tab = this.editorTabFactory.createTab(tabData.tabType, {
-              projectId: tabData.projectId,
-              headerText: tabData.projectDoc?.data?.shortName
-            });
+        targetTabGroup.addTab(
+          await this.editorTabFactory.createTab('project-target', {
+            projectId: projectDoc.id,
+            headerText: projectDoc.data?.shortName
+          })
+        );
 
-            if (tabData.groupId === 'source') {
-              sourceTabGroup.addTab(tab, tabData.isSelected);
-            } else {
-              targetTabGroup.addTab(tab, tabData.isSelected);
-            }
+        persistedTabs.forEach(async tabData => {
+          const tab: EditorTabInfo = await this.editorTabFactory.createTab(tabData.tabType, {
+            projectId: tabData.projectId,
+            headerText: tabData.projectDoc?.data?.shortName
           });
 
-          this.tabState.setTabGroups([sourceTabGroup, targetTabGroup]);
+          if (tabData.groupId === 'source') {
+            sourceTabGroup.addTab(tab, tabData.isSelected);
+          } else {
+            targetTabGroup.addTab(tab, tabData.isSelected);
+          }
+        });
 
-          // Notify to start tab persistence on tab state changes
-          tabStateInitialized$.next(true);
+        this.tabState.setTabGroups([sourceTabGroup, targetTabGroup]);
 
-          // View is initialized before the tab state is initialized, so re-run change detection
-          this.changeDetector.detectChanges();
-        }),
-        take(1)
-      )
-      .subscribe();
+        // Notify to start tab persistence on tab state changes
+        tabStateInitialized$.next(true);
+
+        // View is initialized before the tab state is initialized, so re-run change detection
+        this.changeDetector.detectChanges();
+      });
 
     // Persist tabs from tab state changes once tab state has been initialized
     combineLatest([this.tabState.tabs$, tabStateInitialized$.pipe(filter(initialized => initialized))])
@@ -1332,7 +1331,7 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
     }
   }
 
-  private updateAutoDraftTabVisibility(): void {
+  private async updateAutoDraftTabVisibility(): Promise<void> {
     const chapter: Chapter | undefined = this.text?.chapters.find(c => c.number === this._chapter);
     const hasDraft: boolean = chapter?.hasDraft ?? false;
     const existingDraftTab: { groupId: EditorTabGroupType; index: number } | undefined =
@@ -1344,7 +1343,7 @@ export class EditorComponent extends DataLoadingComponent implements OnDestroy,
 
       // Add to 'source' tab group if no draft tab
       if (existingDraftTab == null) {
-        this.tabState.addTab('source', this.editorTabFactory.createTab('draft'), urlDraftActive);
+        this.tabState.addTab('source', await this.editorTabFactory.createTab('draft'), urlDraftActive);
       }
 
       if (urlDraftActive) {

diff --git a/....Scripture/ClientApp/src/app/translate/editor/tabs/editor-tab-add-request.service.spec.ts b/....Scripture/ClientApp/src/app/translate/editor/tabs/editor-tab-add-request.service.spec.ts
@@ -61,8 +61,8 @@ describe('EditorTabAddRequestService', () => {
     const projectDoc2 = createTestProjectDoc(2);
 
     when(tabStateService.tabs$).thenReturn(of([{ projectId: projectDoc1.id }, {}, { projectId: projectDoc2.id }]));
-    when(projectService.get(projectDoc1.id)).thenReturn(Promise.resolve(projectDoc1));
-    when(projectService.get(projectDoc2.id)).thenReturn(Promise.resolve(projectDoc2));
+    when(projectService.get(projectDoc1.id)).thenResolve(projectDoc1);
+    when(projectService.get(projectDoc2.id)).thenResolve(projectDoc2);
 
     service['getParatextIdsForOpenTabs']().subscribe(result => {
       expect(result).toEqual([projectDoc1.data!.paratextId, projectDoc2.data!.paratextId]);