Merge pull request #1455 from dpalou/MOBILE-2472

MOBILE-2472 course: Fix spinner shown forever when changing format

src/components/dynamic-component/dynamic-component.ts

@@ -67,6 +67,7 @@ export class CoreDynamicComponent implements OnInit, OnChanges, DoCheck {
     container: ViewContainerRef;
     protected logger: any;
     protected differ: any; // To detect changes in the data input.
+    protected lastComponent: any;
 
     constructor(logger: CoreLoggerProvider, protected factoryResolver: ComponentFactoryResolver, differs: KeyValueDiffers,
             @Optional() protected navCtrl: NavController, protected cdr: ChangeDetectorRef, protected element: ElementRef,
@@ -87,7 +88,13 @@ export class CoreDynamicComponent implements OnInit, OnChanges, DoCheck {
      * Detect changes on input properties.
      */
     ngOnChanges(changes: { [name: string]: SimpleChange }): void {
-        if (!this.instance && changes.component) {
+
+        if (changes.component && !this.component) {
+            // Component not set, destroy the instance if any.
+            this.lastComponent = undefined;
+            this.instance = undefined;
+            this.container && this.container.clear();
+        } else if (changes.component && (!this.instance || this.component != this.lastComponent)) {
             this.createComponent();
         }
     }
@@ -127,6 +134,8 @@ export class CoreDynamicComponent implements OnInit, OnChanges, DoCheck {
      * @return {boolean} Whether the component was successfully created.
      */
     protected createComponent(): boolean {
+        this.lastComponent = this.component;
+
         if (!this.component || !this.container) {
             // No component to instantiate or container doesn't exist right now.
             return false;

src/core/course/classes/main-activity-component.ts

@@ -50,8 +50,8 @@ export class CoreCourseModuleMainActivityComponent extends CoreCourseModuleMainR
     protected eventsProvider: CoreEventsProvider;
     protected modulePrefetchDelegate: CoreCourseModulePrefetchDelegate;
 
-    constructor(injector: Injector, protected content?: Content) {
+    constructor(injector: Injector, protected content?: Content, loggerName: string = 'CoreCourseModuleMainResourceComponent') {
-        super(injector);
+        super(injector, loggerName);
 
         this.sitesProvider = injector.get(CoreSitesProvider);
         this.courseProvider = injector.get(CoreCourseProvider);
@@ -120,10 +120,28 @@ export class CoreCourseModuleMainActivityComponent extends CoreCourseModuleMainR
      * @return {Promise<any>} Resolved when done.
      */
     protected refreshContent(sync: boolean = false, showErrors: boolean = false): Promise<any> {
+        if (!this.module) {
+            // This can happen if course format changes from single activity to weekly/topics.
+            return Promise.resolve();
+        }
+
         this.refreshIcon = 'spinner';
         this.syncIcon = 'spinner';
 
-        return this.invalidateContent().catch(() => {
+        // Wrap the call in a try/catch so the workflow isn't interrupted if an error occurs.
+        // E.g. when changing course format we cannot know when will this.module become undefined, so it could cause errors.
+        let promise;
+
+        try {
+            promise = this.invalidateContent();
+        } catch (ex) {
+            // An error ocurred in the function, log the error and just resolve the promise so the workflow continues.
+            this.logger.error(ex);
+
+            promise = Promise.resolve();
+        }
+
+        return promise.catch(() => {
             // Ignore errors.
         }).then(() => {
             return this.loadContent(true, sync, showErrors);
@@ -191,7 +209,25 @@ export class CoreCourseModuleMainActivityComponent extends CoreCourseModuleMainR
     protected loadContent(refresh?: boolean, sync: boolean = false, showErrors: boolean = false): Promise<any> {
         this.isOnline = this.appProvider.isOnline();
 
-        return this.fetchContent(refresh, sync, showErrors).catch((error) => {
+        if (!this.module) {
+            // This can happen if course format changes from single activity to weekly/topics.
+            return Promise.resolve();
+        }
+
+        // Wrap the call in a try/catch so the workflow isn't interrupted if an error occurs.
+        // E.g. when changing course format we cannot know when will this.module become undefined, so it could cause errors.
+        let promise;
+
+        try {
+            promise = this.fetchContent(refresh, sync, showErrors);
+        } catch (ex) {
+            // An error ocurred in the function, log the error and just resolve the promise so the workflow continues.
+            this.logger.error(ex);
+
+            promise = Promise.resolve();
+        }
+
+        return promise.catch((error) => {
             if (!refresh) {
                 // Some call failed, retry without using cache since it might be a new activity.
                 return this.refreshContent(sync);

src/core/course/classes/main-resource-component.ts

@@ -14,6 +14,7 @@
 
 import { OnInit, OnDestroy, Input, Output, EventEmitter, Injector } from '@angular/core';
 import { TranslateService } from '@ngx-translate/core';
+import { CoreLoggerProvider } from '@providers/logger';
 import { CoreDomUtilsProvider } from '@providers/utils/dom';
 import { CoreTextUtilsProvider } from '@providers/utils/text';
 import { CoreCourseHelperProvider } from '@core/course/providers/helper';
@@ -54,7 +55,9 @@ export class CoreCourseModuleMainResourceComponent implements OnInit, OnDestroy,
     protected moduleDelegate: CoreCourseModuleDelegate;
     protected courseSectionPage: CoreCourseSectionPage;
 
-    constructor(injector: Injector) {
+    protected logger;
+
+    constructor(injector: Injector, loggerName: string = 'CoreCourseModuleMainResourceComponent') {
         this.textUtils = injector.get(CoreTextUtilsProvider);
         this.courseHelper = injector.get(CoreCourseHelperProvider);
         this.translate = injector.get(TranslateService);
@@ -62,6 +65,9 @@ export class CoreCourseModuleMainResourceComponent implements OnInit, OnDestroy,
         this.moduleDelegate = injector.get(CoreCourseModuleDelegate);
         this.courseSectionPage = injector.get(CoreCourseSectionPage, null);
         this.dataRetrieved = new EventEmitter();
+
+        const loggerProvider = injector.get(CoreLoggerProvider);
+        this.logger = loggerProvider.getInstance(loggerName);
     }
 
     /**
@@ -84,7 +90,7 @@ export class CoreCourseModuleMainResourceComponent implements OnInit, OnDestroy,
      * @return {Promise<any>} Promise resolved when done.
      */
     doRefresh(refresher?: any, done?: () => void, showErrors: boolean = false): Promise<any> {
-        if (this.loaded) {
+        if (this.loaded && this.module) {
             /* If it's a single activity course and the refresher is displayed within the component,
                call doRefresh on the section page to refresh the course data. */
             let promise;
@@ -113,9 +119,27 @@ export class CoreCourseModuleMainResourceComponent implements OnInit, OnDestroy,
      * @return {Promise<any>} Resolved when done.
      */
      protected refreshContent(sync: boolean = false, showErrors: boolean = false): Promise<any> {
+        if (!this.module) {
+            // This can happen if course format changes from single activity to weekly/topics.
+            return Promise.resolve();
+        }
+
         this.refreshIcon = 'spinner';
 
-        return this.invalidateContent().catch(() => {
+        // Wrap the call in a try/catch so the workflow isn't interrupted if an error occurs.
+        // E.g. when changing course format we cannot know when will this.module become undefined, so it could cause errors.
+        let promise;
+
+        try {
+            promise = this.invalidateContent();
+        } catch (ex) {
+            // An error ocurred in the function, log the error and just resolve the promise so the workflow continues.
+            this.logger.error(ex);
+
+            promise = Promise.resolve();
+        }
+
+        return promise.catch(() => {
             // Ignore errors.
         }).then(() => {
             return this.loadContent(true);
@@ -150,6 +174,24 @@ export class CoreCourseModuleMainResourceComponent implements OnInit, OnDestroy,
      * @return {Promise<any>} Promise resolved when done.
      */
     protected loadContent(refresh?: boolean): Promise<any> {
+        if (!this.module) {
+            // This can happen if course format changes from single activity to weekly/topics.
+            return Promise.resolve();
+        }
+
+        // Wrap the call in a try/catch so the workflow isn't interrupted if an error occurs.
+        // E.g. when changing course format we cannot know when will this.module become undefined, so it could cause errors.
+        let promise;
+
+        try {
+            promise = this.fetchContent(refresh);
+        } catch (ex) {
+            // An error ocurred in the function, log the error and just resolve the promise so the workflow continues.
+            this.logger.error(ex);
+
+            promise = Promise.resolve();
+        }
+
         return this.fetchContent(refresh).catch((error) => {
             // Error getting data, fail.
             this.domUtils.showErrorModalDefault(error, this.fetchContentDefaultError, true);

src/core/course/components/format/format.ts

@@ -68,6 +68,7 @@ export class CoreCourseFormatComponent implements OnInit, OnChanges, OnDestroy {
     loaded: boolean;
 
     protected sectionStatusObserver;
+    protected lastCourseFormat: string;
 
     constructor(private cfDelegate: CoreCourseFormatDelegate, translate: TranslateService, private injector: Injector,
             private courseHelper: CoreCourseHelperProvider, private domUtils: CoreDomUtilsProvider,
@@ -197,32 +198,29 @@ export class CoreCourseFormatComponent implements OnInit, OnChanges, OnDestroy {
      * Get the components classes.
      */
     protected getComponents(): void {
-        if (this.course) {
+        if (this.course && this.course.format != this.lastCourseFormat) {
-            if (!this.courseFormatComponent) {
+            this.lastCourseFormat = this.course.format;
-                this.cfDelegate.getCourseFormatComponent(this.injector, this.course).then((component) => {
+
-                    this.courseFormatComponent = component;
+            // Format has changed or it's the first time, load all the components.
-                });
+            this.cfDelegate.getCourseFormatComponent(this.injector, this.course).then((component) => {
-            }
+                this.courseFormatComponent = component;
-            if (!this.courseSummaryComponent) {
+            });
-                this.cfDelegate.getCourseSummaryComponent(this.injector, this.course).then((component) => {
+
-                    this.courseSummaryComponent = component;
+            this.cfDelegate.getCourseSummaryComponent(this.injector, this.course).then((component) => {
-                });
+                this.courseSummaryComponent = component;
-            }
+            });
-            if (!this.sectionSelectorComponent) {
+
-                this.cfDelegate.getSectionSelectorComponent(this.injector, this.course).then((component) => {
+            this.cfDelegate.getSectionSelectorComponent(this.injector, this.course).then((component) => {
-                    this.sectionSelectorComponent = component;
+                this.sectionSelectorComponent = component;
-                });
+            });
-            }
+
-            if (!this.singleSectionComponent) {
+            this.cfDelegate.getSingleSectionComponent(this.injector, this.course).then((component) => {
-                this.cfDelegate.getSingleSectionComponent(this.injector, this.course).then((component) => {
+                this.singleSectionComponent = component;
-                    this.singleSectionComponent = component;
+            });
-                });
+
-            }
+            this.cfDelegate.getAllSectionsComponent(this.injector, this.course).then((component) => {
-            if (!this.allSectionsComponent) {
+                this.allSectionsComponent = component;
-                this.cfDelegate.getAllSectionsComponent(this.injector, this.course).then((component) => {
+            });
-                    this.allSectionsComponent = component;
-                });
-            }
         }
     }
 

src/core/course/components/unsupported-module/core-course-unsupported-module.html

@@ -1,5 +1,5 @@
 <div padding>
-    <core-course-module-description [description]="module.description"></core-course-module-description>
+    <core-course-module-description [description]="module && module.description"></core-course-module-description>
     <h2 *ngIf="!isDisabledInSite && isSupportedByTheApp">{{ 'core.whoops' | translate }}</h2>
     <h2 *ngIf="isDisabledInSite || !isSupportedByTheApp">{{ 'core.uhoh' | translate }}</h2>
 
@@ -8,7 +8,7 @@
     <p class="core-big" *ngIf="!isDisabledInSite && !isSupportedByTheApp">{{ 'core.course.activitynotyetviewableremoteaddon' | translate }}</p>
     <p *ngIf="isDisabledInSite || !isSupportedByTheApp"><strong>{{ 'core.course.askadmintosupport' | translate }}</strong></p>
 
-    <div *ngIf="module.url">
+    <div *ngIf="module && module.url">
         <p><strong>{{ 'core.course.useactivityonbrowser' | translate }}</strong></p>
         <a ion-button block icon-end [href]="module.url" core-link>
             {{ 'core.openinbrowser' | translate }}