[Global Pins] Saves all currently pinned cards if the user has enabled global pins. (#6848)

## Motivation for features / changes

Feature request during the teamfood: `Clicking disable pins -> pinning a
card -> clicking enable pins` doesn't save the currently pinned card. It
seems reasonable to save all currently pinned cards when the setting is
activated.

This PR saves all currently pinned cards when user clicks enable global
pins.

## Technical description of changes

* 97c6324 Added `saveScalarPins` to the
data source

* c6be93b Modified `addOrRemovePinsOnToggle` logic to include adding
pins logic
* Previously, if a user disabled the global pin feature, all pins would
be removed.
* This PR added logic that will cause all currently pinned cards to be
saved to local storage when the user activates the feature.

## Screenshots of UI changes (or N/A)
N/A
## Detailed steps to verify changes work correctly (as executed by you)
Unit test passes
## Alternate designs / implementations considered (or N/A)
N/A

Author: roseayeon

tensorboard/webapp/metrics/data_source/saved_pins_data_source.ts

@@ -30,6 +30,16 @@ export class SavedPinsDataSource {
     );
   }
 
+  saveScalarPins(tags: Tag[]): void {
+    const existingPins = this.getSavedScalarPins();
+    const newTags = tags.filter((v) => !existingPins.includes(v));
+    existingPins.push(...newTags);
+    window.localStorage.setItem(
+      SAVED_SCALAR_PINS_KEY,
+      JSON.stringify(existingPins)
+    );
+  }
+
   removeScalarPin(tag: Tag): void {
     const existingPins = this.getSavedScalarPins();
     window.localStorage.setItem(

tensorboard/webapp/metrics/data_source/saved_pins_data_source_test.ts

@@ -85,7 +85,7 @@ describe('SavedPinsDataSource Test', () => {
       expect(dataSource.getSavedScalarPins()).toEqual(['tag1', 'tag2']);
     });
 
-    it('does not addd the provided tag if it already exists', () => {
+    it('does not add the provided tag if it already exists', () => {
       window.localStorage.setItem(
         SAVED_SCALAR_PINS_KEY,
         JSON.stringify(['tag1', 'tag2'])
@@ -97,6 +97,36 @@ describe('SavedPinsDataSource Test', () => {
     });
   });
 
+  describe('saveScalarPins', () => {
+    it('stores the provided tags in the local storage', () => {
+      dataSource.saveScalarPins(['tag1', 'tag2']);
+
+      expect(dataSource.getSavedScalarPins()).toEqual(['tag1', 'tag2']);
+    });
+
+    it('adds the provided tags to the existing list', () => {
+      window.localStorage.setItem(
+        SAVED_SCALAR_PINS_KEY,
+        JSON.stringify(['tag1'])
+      );
+
+      dataSource.saveScalarPins(['tag2']);
+
+      expect(dataSource.getSavedScalarPins()).toEqual(['tag1', 'tag2']);
+    });
+
+    it('does not add the tag if it already exists', () => {
+      window.localStorage.setItem(
+        SAVED_SCALAR_PINS_KEY,
+        JSON.stringify(['tag1', 'tag2'])
+      );
+
+      dataSource.saveScalarPins(['tag2', 'tag3']);
+
+      expect(dataSource.getSavedScalarPins()).toEqual(['tag1', 'tag2', 'tag3']);
+    });
+  });
+
   describe('removeScalarPin', () => {
     it('removes the given tag if it exists', () => {
       dataSource.saveScalarPin('tag3');

tensorboard/webapp/metrics/effects/index.ts

@@ -43,6 +43,7 @@ import {
   TimeSeriesRequest,
   TimeSeriesResponse,
   SavedPinsDataSource,
+  Tag,
 } from '../data_source/index';
 import {
   getCardLoadState,
@@ -356,26 +357,29 @@ export class MetricsEffects implements OnInitEffects {
     })
   );
 
-  private readonly removeSavedPinsOnDisable$ = this.actions$.pipe(
+  private readonly addOrRemovePinsOnToggle$ = this.actions$.pipe(
     ofType(actions.metricsEnableSavingPinsToggled),
     withLatestFrom(
+      this.store.select(selectors.getPinnedCardsWithMetadata),
       this.store.select(selectors.getEnableGlobalPins),
       this.store.select(selectors.getShouldPersistSettings),
       this.store.select(selectors.getMetricsSavingPinsEnabled)
     ),
     filter(
-      ([
-        ,
-        enableGlobalPins,
-        getShouldPersistSettings,
-        getMetricsSavingPinsEnabled,
-      ]) =>
-        enableGlobalPins &&
-        getShouldPersistSettings &&
-        !getMetricsSavingPinsEnabled
+      ([, , enableGlobalPins, getShouldPersistSettings]) =>
+        enableGlobalPins && getShouldPersistSettings
     ),
-    tap(() => {
-      this.savedPinsDataSource.removeAllScalarPins();
+    tap(([, pinnedCards, , , getMetricsSavingPinsEnabled]) => {
+      if (getMetricsSavingPinsEnabled) {
+        const tags: Tag[] = pinnedCards
+          .map((card) => {
+            return card.plugin === PluginType.SCALARS ? card.tag : null;
+          })
+          .filter((v): v is Tag => v !== null);
+        this.savedPinsDataSource.saveScalarPins(tags);
+      } else {
+        this.savedPinsDataSource.removeAllScalarPins();
+      }
     })
   );
 
@@ -427,7 +431,7 @@ export class MetricsEffects implements OnInitEffects {
         /**
          * Subscribes to: metricsEnableSavingPinsToggled.
          */
-        this.removeSavedPinsOnDisable$
+        this.addOrRemovePinsOnToggle$
       );
     },
     {dispatch: false}

tensorboard/webapp/metrics/effects/metrics_effects_test.ts

@@ -45,6 +45,7 @@ import {
   appStateFromMetricsState,
   buildDataSourceTagMetadata,
   buildMetricsState,
+  createCardMetadata,
   createScalarStepData,
   provideTestingMetricsDataSource,
   provideTestingSavedPinsDataSource,
@@ -1061,50 +1062,73 @@ describe('metrics effects', () => {
       });
     });
 
-    describe('removeSavedPinsOnDisable', () => {
+    describe('addOrRemovePinsOnToggle', () => {
       let removeAllScalarPinsSpy: jasmine.Spy;
+      let saveScalarPinsSpy: jasmine.Spy;
 
       beforeEach(() => {
         removeAllScalarPinsSpy = spyOn(
           savedPinsDataSource,
           'removeAllScalarPins'
         );
+        saveScalarPinsSpy = spyOn(savedPinsDataSource, 'saveScalarPins');
+        store.overrideSelector(selectors.getPinnedCardsWithMetadata, [
+          {
+            cardId: 'card1',
+            ...createCardMetadata(PluginType.SCALARS),
+            tag: 'tag1',
+          },
+          {
+            cardId: 'card2',
+            ...createCardMetadata(PluginType.IMAGES),
+            tag: 'tag2',
+          },
+          {
+            cardId: 'card3',
+            ...createCardMetadata(PluginType.SCALARS),
+            tag: 'tag3',
+          },
+        ]);
         store.overrideSelector(selectors.getEnableGlobalPins, true);
         store.overrideSelector(selectors.getMetricsSavingPinsEnabled, false);
         store.refreshState();
       });
 
-      it('removes all pins by calling metricsEnableSavingPinsToggled method', () => {
+      it('removes all pins if getMetricsSavingPinsEnabled is false', () => {
         actions$.next(actions.metricsEnableSavingPinsToggled());
 
         expect(removeAllScalarPinsSpy).toHaveBeenCalled();
+        expect(saveScalarPinsSpy).not.toHaveBeenCalled();
       });
 
-      it('does not remove pins if getEnableGlobalPins is false', () => {
-        store.overrideSelector(selectors.getEnableGlobalPins, false);
+      it('add existing pins if getMetricsSavingPinsEnabled is true', () => {
+        store.overrideSelector(selectors.getMetricsSavingPinsEnabled, true);
         store.refreshState();
 
         actions$.next(actions.metricsEnableSavingPinsToggled());
 
+        expect(saveScalarPinsSpy).toHaveBeenCalledWith(['tag1', 'tag3']);
         expect(removeAllScalarPinsSpy).not.toHaveBeenCalled();
       });
 
-      it('does not remove pins if getShouldPersistSettings is false', () => {
-        store.overrideSelector(selectors.getShouldPersistSettings, false);
+      it('does not add or remove pins if getEnableGlobalPins is false', () => {
+        store.overrideSelector(selectors.getEnableGlobalPins, false);
         store.refreshState();
 
         actions$.next(actions.metricsEnableSavingPinsToggled());
 
         expect(removeAllScalarPinsSpy).not.toHaveBeenCalled();
+        expect(saveScalarPinsSpy).not.toHaveBeenCalled();
       });
 
-      it('does not remove pins if getMetricsSavingPinsEnabled is true', () => {
-        store.overrideSelector(selectors.getMetricsSavingPinsEnabled, true);
+      it('does not add or remove pins if getShouldPersistSettings is false', () => {
+        store.overrideSelector(selectors.getShouldPersistSettings, false);
         store.refreshState();
 
         actions$.next(actions.metricsEnableSavingPinsToggled());
 
         expect(removeAllScalarPinsSpy).not.toHaveBeenCalled();
+        expect(saveScalarPinsSpy).not.toHaveBeenCalled();
       });
     });
   });

tensorboard/webapp/metrics/testing.ts

@@ -422,6 +422,8 @@ export function buildStepIndexMetadata(
 export class TestingSavedPinsDataSource {
   saveScalarPin(tag: Tag) {}
 
+  saveScalarPins(tag: Tag[]) {}
+
   removeScalarPin(tag: Tag) {}
 
   getSavedScalarPins() {