🐛Convert arg 'index' to number. (#26533)

* Convert arg 'index' to number. * switch to userAssert and add unit tests
ampproject · Jan 31, 2020 · 307c885 · 307c885
1 parent dd33fa5
commit 307c885
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 2 deletions.
diff --git a/extensions/amp-carousel/0.2/amp-carousel.js b/extensions/amp-carousel/0.2/amp-carousel.js
@@ -24,7 +24,7 @@ import {Services} from '../../../src/services';
 import {closestAncestorElementBySelector} from '../../../src/dom';
 import {computedStyle} from '../../../src/style';
 import {createCustomEvent, getDetail} from '../../../src/event-helper';
-import {dev, devAssert} from '../../../src/log';
+import {dev, devAssert, userAssert} from '../../../src/log';
 import {dict} from '../../../src/utils/object';
 import {htmlFor} from '../../../src/static-template';
 import {isLayoutSizeDefined} from '../../../src/layout';
@@ -47,7 +47,14 @@ class AmpCarousel extends AMP.BaseElement {
       'goToSlide',
       actionInvocation => {
         const {args, trust} = actionInvocation;
-        this.carousel_.goToSlide(args['index'] || 0, {
+        const slide = Number(args['index'] || 0);
+        userAssert(
+          !isNaN(slide),
+          'Unexpected slide index for goToSlide action: %s. %s',
+          args['index'],
+          this.element
+        );
+        this.carousel_.goToSlide(slide, {
           actionSource: this.getActionSource_(trust),
         });
       },

diff --git a/extensions/amp-carousel/0.2/test/test-type-slides.js b/extensions/amp-carousel/0.2/test/test-type-slides.js
@@ -264,6 +264,49 @@ describes.realWin(
           ActionTrust.LOW
         );
       });
+
+      it('should allow string-valued index', async () => {
+        const carousel = await getCarousel({loop: false});
+        const impl = carousel.implementation_;
+        const triggerSpy = env.sandbox.spy(impl.action_, 'trigger');
+
+        impl.executeAction({
+          method: 'goToSlide',
+          args: {index: '1'},
+          trust: ActionTrust.LOW,
+          satisfiesTrust: () => true,
+        });
+        await afterIndexUpdate(carousel);
+
+        expect(triggerSpy).to.have.been.calledWith(
+          carousel,
+          'slideChange',
+          /* CustomEvent */ env.sandbox.match.has('detail', {index: 1}),
+          ActionTrust.LOW
+        );
+      });
+
+      it('should cause error with invalid index', async () => {
+        const carousel = await getCarousel({loop: false});
+        const impl = carousel.implementation_;
+        const triggerSpy = env.sandbox.spy(impl.action_, 'trigger');
+
+        try {
+          allowConsoleError(() => {
+            impl.executeAction({
+              method: 'goToSlide',
+              args: {index: 'one'},
+              trust: ActionTrust.LOW,
+              satisfiesTrust: () => true,
+            });
+          });
+          await afterIndexUpdate(carousel);
+        } catch (expected) {
+          expect(triggerSpy).to.not.have.been.called;
+          return;
+        }
+        expect.fail();
+      });
     });
 
     describe('layout direction', () => {