Refactor surveys and add a new one #4794

DavidKutu · 2021-02-12T22:24:43Z

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
Has a news entry file (remember to thank yourself!).
Appropriate comments and documentation strings in the code.
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated.
Test plan is updated as appropriate.
package-lock.json has been regenerated by running npm install (if dependencies have changed).

rchiodo · 2021-02-12T22:40:02Z

src/client/datascience/dataScienceSurveyBanner.ts

    private bannerLabels: string[] = [
        localize.DataScienceSurveyBanner.bannerLabelYes(),
        localize.DataScienceSurveyBanner.bannerLabelNo()
    ];
-    private readonly showBannerState: IPersistentState<ShowBannerWithExpiryTime>;
-    private readonly surveyLink: string;
+    private readonly showBannerState: IPersistentState<ShowBannerWithExpiryTime>[] = [];


Seems like this should be a map, not an array. It would reenforce that it is being set correctly.

Suggested change

private readonly showBannerState: IPersistentState<ShowBannerWithExpiryTime>[] = [];

private readonly showBannerState = new Map<BannerType, IPersistentState<ShowBannerWithExpiryTime>>();

rchiodo · 2021-02-12T22:43:11Z

src/client/datascience/dataScienceSurveyBanner.ts

-        const state = this.persistentState.createGlobalPersistentState<number>(DSSurveyStateKeys.OpenNotebookCount, 0);
-        await state.updateValue(state.value + 1);
-        return this.showBanner();
+        const state1 = this.persistentState.createGlobalPersistentState<number>(DSSurveyStateKeys.OpenNotebookCount, 0);


This seems like you could call the same function with a different string 3 times to update the state for all 3.

private updateStateAndShowBanner(val: string, banner: BannerType) { const state = this.persistentState.createGlobalPersistentState<number>(val, 0); await state.updateValue(state.value + 1); this.showBanner(banner); }

yes, I'll also do that for getOpenNotebookCount and getExecutionCount

rchiodo · 2021-02-12T22:43:34Z

src/client/datascience/dataScienceSurveyBanner.ts

+                InsidersNotebookSurveyStateKeys.ExecutionCount,
+                0
+            );
+            await state1.updateValue(state1.value + 1);


That function I mentioned above would be called here.

IanMatthewHuff · 2021-02-12T23:09:49Z

src/client/datascience/dataScienceSurveyBanner.ts

+                }
+                break;
+            case BannerType.DSSurvey:
+            default:


Just a minor thing here. I'd break off default here and have it traceError and return false. isEnabled should always be called with a BannerType, so if you want the DSSurvey you should specifically request it. If a new BannerType is added we want someone to explicitly add a new case here, now with default it would go down the DSSurvey path if you forgot to add the case, which you don't want.

just isEnabled? or all of the switches?

Yeah, you don't want a default like that here. Only if it has a legit default that you want to happen.

codecov-io · 2021-02-12T23:57:35Z

Codecov Report

Merging #4794 (ab152f6) into main (93d6475) will decrease coverage by 2%.
The diff coverage is 66%.

@@          Coverage Diff           @@
##            main   #4794    +/-   ##
======================================
- Coverage     76%     74%    -3%     
======================================
  Files        411     410     -1     
  Lines      26963   26939    -24     
  Branches    3909    3902     -7     
======================================
- Hits       20741   20065   -676     
- Misses      4611    5304   +693     
+ Partials    1611    1570    -41

Impacted Files	Coverage Δ
src/client/common/types.ts	`100% <ø> (ø)`
src/client/datascience/serviceRegistry.ts	`98% <ø> (-1%)`	⬇️
src/client/datascience/dataScienceSurveyBanner.ts	`65% <65%> (-24%)`	⬇️
src/client/activation/serviceRegistry.ts	`100% <100%> (ø)`
src/client/datascience/notebook/introStartPage.ts	`100% <100%> (ø)`
src/client/datascience/shiftEnterBanner.ts	`91% <100%> (ø)`
...ience/variablesView/variableViewMessageListener.ts	`22% <0%> (-78%)`	⬇️
...ent/common/application/webviewViews/webviewView.ts	`14% <0%> (-72%)`	⬇️
...t/common/application/webviewPanels/webviewPanel.ts	`10% <0%> (-63%)`	⬇️
src/client/common/application/webviews/webview.ts	`13% <0%> (-60%)`	⬇️
... and 89 more

rchiodo

⏲️

* Refactor surveys and add a new one * add news and lint * lint * lint * PR comments

* Refactor surveys and add a new one (#4794) * Refactor surveys and add a new one * add news and lint * lint * lint * PR comments * update changelog * Fix IJupyterExtensionBanner service identifier (#4825) Co-authored-by: Joyce Er <joyceerhuiling@gmail.com>

* PORT Variable view icon to release branch (#4748) * update changelog (#4752) * Cherry pick change for icon overflow (#4753) * Change number of icons in notebook toolbar (#4747) * Fix icons * Have trust command show up and nothing else if untrusted * Add trust check to all commands * Update based on discussion * Update change log * PORT - variable context (#4770) * Add new editor context for just if native notebook is active (#4762) * update news Co-authored-by: Ian Huff <ianhuff@LICHYOGAIOT.northamerica.corp.microsoft.com> * Port control enter fix to release (#4774) * Fix ctrl+enter and alt+enter by removing the redundant command. (#4769) * Fix ctrl+enter and alt+enter by removing the redundant command. * Code review feedback * Update changelog * port .NET fix to release (#4776) * Fix .NET interactive to always use native (#4772) * Fix .NET interactive * Fix tests and linter * Update changelog * Port kernel finder change to release (#4799) * Fix duplicate kernel generation (#4790) * Fix duplicate kernel generation * Fix functional tests * Update changelog * Actually use stable * PORT - Port document dispose fix to release. (#4798) * Only clean up editors on close not on Panel dispose (#4787) * update changelog Co-authored-by: Ian Huff <ianhuff@LICHYOGAIOT.northamerica.corp.microsoft.com> * Port surveys (#4804) * Refactor surveys and add a new one (#4794) * Refactor surveys and add a new one * add news and lint * lint * lint * PR comments * update changelog * Fix IJupyterExtensionBanner service identifier (#4825) Co-authored-by: Joyce Er <joyceerhuiling@gmail.com> * PORT - Fix for top level save and undo commands (#4851) * Update cache ignore * Actually ignore .so files * Update release plan * Put back package.json changes * Delete news entries Co-authored-by: Ian Huff <ianhu@microsoft.com> Co-authored-by: David Kutugata <dakutuga@microsoft.com> Co-authored-by: Ian Huff <ianhuff@LICHYOGAIOT.northamerica.corp.microsoft.com> Co-authored-by: Joyce Er <joyceerhuiling@gmail.com>

Refactor surveys and add a new one

3fa4ead

DavidKutu requested a review from a team as a code owner February 12, 2021 22:24

David Kutugata added 3 commits February 12, 2021 14:29

add news and lint

485c75c

lint

e8c49c4

lint

db091bb

rchiodo reviewed Feb 12, 2021

View reviewed changes

IanMatthewHuff reviewed Feb 12, 2021

View reviewed changes

PR comments

ab152f6

DavidKutu requested review from IanMatthewHuff and rchiodo February 12, 2021 23:59

IanMatthewHuff approved these changes Feb 13, 2021

View reviewed changes

rchiodo requested changes Feb 13, 2021

View reviewed changes

rchiodo approved these changes Feb 13, 2021

View reviewed changes

Merge branch 'main' into david/updateSurveys

aaafc00

DavidKutu merged commit 88d1d0f into main Feb 13, 2021

DavidKutu deleted the david/updateSurveys branch February 13, 2021 02:20

DavidKutu pushed a commit that referenced this pull request Feb 13, 2021

Refactor surveys and add a new one (#4794)

a413a82

* Refactor surveys and add a new one * add news and lint * lint * lint * PR comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor surveys and add a new one #4794

Refactor surveys and add a new one #4794

DavidKutu commented Feb 12, 2021

rchiodo Feb 12, 2021

rchiodo Feb 12, 2021

DavidKutu Feb 12, 2021

rchiodo Feb 12, 2021

IanMatthewHuff Feb 12, 2021

DavidKutu Feb 12, 2021

IanMatthewHuff Feb 12, 2021

codecov-io commented Feb 12, 2021 •

edited

Loading

rchiodo left a comment

	private readonly showBannerState: IPersistentState<ShowBannerWithExpiryTime>[] = [];
	private readonly showBannerState = new Map<BannerType, IPersistentState<ShowBannerWithExpiryTime>>();

Refactor surveys and add a new one #4794

Refactor surveys and add a new one #4794

Conversation

DavidKutu commented Feb 12, 2021

rchiodo Feb 12, 2021

Choose a reason for hiding this comment

rchiodo Feb 12, 2021

Choose a reason for hiding this comment

DavidKutu Feb 12, 2021

Choose a reason for hiding this comment

rchiodo Feb 12, 2021

Choose a reason for hiding this comment

IanMatthewHuff Feb 12, 2021

Choose a reason for hiding this comment

DavidKutu Feb 12, 2021

Choose a reason for hiding this comment

IanMatthewHuff Feb 12, 2021

Choose a reason for hiding this comment

codecov-io commented Feb 12, 2021 • edited Loading

Codecov Report

rchiodo left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 12, 2021 •

edited

Loading