[Feature] Ratings prompt #22

iconix · 2016-09-12T20:47:58Z

This feature enables the Web Clipper to receive positive and negative feedback from "active" users.

When a user's engagement with the Web Clipper qualifies them, we will show an additional sub-panel after a successful clip, which asks: "Enjoying the Web Clipper?" We will then log their response ("Yes, it's great!" vs "Not really...") depending on button clicked.

Additionally, if rate and feedback URLs are provided in Web Clipper settings, we will show the user additional prompts that will direct them to places where they can leave further feedback.

The "when to show" logic for this ratings sub-panel is where things get a little complicated. In broad strokes, the checks break down as follows:

Is this feature enabled for the client being used (e.g., ChromeExtension, EdgeExtension, Bookmarklet, etc.)?
Is the user "active" (dependent on # successful clips)?
Have we shown this ratings panel to the user before? If so, what kind of response did we get (no response, positive response, negative response)?

Please see method descriptions in RatingsHelper.ts for further breakdown of the "when to show" logic.

PENDING TO-DOs as of initial PR publish:

Animations, as spec'd by the design team
Fix commented-out tests in ratingsHelper_tests.tsx
Finalize logic on 1) how to handle users who never interact with the ratings prompt, and 2) how to determine enough time has passed after negative feedback to re-prompt users? (email discussion ongoing)

…or renaming

…he client

…lper

VishaalK · 2016-09-13T16:15:15Z

src/scripts/constants.ts

-		export var noOpTrackerTimeoutDuration = 20 * 1000; // 20 seconds
+		export var fontSizeStep = 2;
+		export var maxClipSuccessForRatingsPrompt = 12; // TODO waiting for consensus on this value
+		export var maximumJSTimeValue = 8640000000000000; // http://ecma-international.org/ecma-262/5.1/#sec-15.9.1.1


can you make this multiplied out like the others?

sorry, what would the multiplication you're suggesting here be? e.g., do you find 100,000,000 (days) * 86,400,000 (ms) more readable (as described by the spec linked to in the comment)?

Eh I guess it doesn't matter since its a max time value.

100000000 * 24 * 60 * 60 * 1000; // 100K days in milliseconds, http://ecma-international.org/ecma-262/5.1/#sec-15.9.1.1

hm, ok I can do that!

#Pending -> #Resolved

…ipper.tsx; increment as part of SaveToOneNote.startClip

…; fix small bugs

VishaalK · 2016-09-19T20:54:51Z

src/tests/settings_tests.ts

@@ -52,10 +52,11 @@ test("helper function checkValueAndDescriptionOnObject should assert when given
 	ok(checkValueAndDescriptionOnObject(valid));
 });

-test("All entries in setting.json should have a non-empty Value and Description", () => {
+// TODO is a non-empty check necessary?
+/*test("All entries in setting.json should have a non-empty Value and Description", () => {


Commented out test?

Yeah, wasn't sure if there was a technical reason why we disallow empty values... because now I'd like to start using empty values. I'll assume it's all good and delete this test.

#Pending -> #Resolved

VishaalK · 2016-09-19T20:57:55Z

src/tests/clipperUI/ratingsHelper_tests.tsx

+
+// MOCK STORAGE
+
+let mockStorage: { [key: string]: string };


We have 3-ish versions of mockStorage out there. I see one in tooltipHelper_tests.ts, another in sectionPicker_tests.ts. Can you add a low pri PBI to unify these? Not necessary for this PR (may never be necessary 😜).

#Resolved (opened #41)

VishaalK · 2016-09-19T21:03:56Z

src/scripts/clipperUI/ratingsHelper.ts

+	 * Public for testing
+	 */
+	public static clipSuccessDelayIsOver(numClips: number): boolean {
+		// TODO MVP+? when # of successful clips > nMax, collapse panel into a Rate Us hyperlink in the footer that is always available


similar to another comment. Create a task for this somewhere

#Resolved (in VSO)

VishaalK · 2016-09-19T21:09:24Z

src/tests/clipperUI/ratingsHelper_tests.tsx

+	strictEqual(shouldShowRatingsPrompt, false);
+});
+
+// test("", () => { });


Tests for RatingsHelper looks very thorough !

VishaalK · 2016-09-19T21:41:02Z

src/scripts/clipperUI/panels/ratingsPanel.tsx

+					});
+				}
+				break;
+			case RatingsPromptStage.Feedback:


Can you split these into functions for each state for readability? The logic looks good though.

#WontFix after in-person discussion: any readability concern seems to apply only to the Init stage functions, and overall isn't too concerning

VishaalK · 2016-09-19T21:58:08Z

src/scripts/clipperUI/frontEndGlobals.ts

+
+		export function preCacheValues(storageKeys: string[]): void {
+			for (let key of storageKeys) {
+				Clipper.Storage.getValue(key, () => { }, true);


I know Matthew made a getValues but you'd have to add in the caching in again that you already added to getValue (if we want to switch to that function). Just a heads up depending on who merges first.

#Pending -> #Resolved (merged with master)

VishaalK · 2016-09-19T21:58:17Z

src/scripts/clipperUI/ratingsHelper.ts

+		let doNotPromptRatingsStr: string = Clipper.Storage.getCachedValue(Constants.StorageKeys.doNotPromptRatings);
+		let lastBadRatingDateAsStr: string = Clipper.Storage.getCachedValue(Constants.StorageKeys.lastBadRatingDate);
+		let lastBadRatingVersion: string = Clipper.Storage.getCachedValue(Constants.StorageKeys.lastBadRatingVersion);
+		let lastSeenVersion: string = Clipper.Storage.getCachedValue(Constants.StorageKeys.lastSeenVersion);


This is gonna sound crappy but I am wary of this paradigm of knowing when a value is cached versus when it isn't. I think since you have tested it pretty thoroughly it will work here. The options are:

Always get the fresh value from localStorage, which means dealing with callbacks, which is why we switched to this design in the first place

Risk not having a fresh value. If the caller needs a fresh value, force them to use the async version.

What happens when you attempt getCachedValue but a corresponding getValue was never called? It is hard to differentiate between not being in storage, as we have a lot of cases where it doesn't exist yet, versus it not being in the cache. From preCacheNeededValues I know that these values are fine but we should discuss how we want to treat this in the future.

TL;DR: async makes everything difficult -.-

Time to take another hard look at being able to use async/await?

^ I think we should too. I think there's lots of benefit in this.

@VishaalK I definitely get your concern here - getCachedValue is easy to accidentally misuse.

@VishaalK @leakymemory @mttwc I assume we're okay with using this paradigm for this PR, even if we need a better permanent solution... so how should we facilitate discussing the future?

Looks like async/await didn't make it into ES7 (ES2016), and only "has a chance" of making ES8 (ES2017)... so do we want to polyfill it? I can open a task for investigation if we all agree.

And what happened with the first hard look at async/await?

#Pending -> #Resolved (opened #43)

iconix · 2016-09-19T23:08:27Z

Noting a bug that @VishaalK informed me of: height-animator doesn't expand out enough anymore when on the SignInNeeded Panel (sign out of the clipper to see that the footer is cut off some - Feedback link is too close to the bottom)

#Pending -> #Resolved

… on the SignInPanel; fix what that broke on RatingsPanel

…lues

…e/ratings-prompt

iconix added 28 commits August 31, 2016 12:26

Add ratings sub-panel to clip success panel

0ac5fb3

Refactor ratings prompt style from inline to stylesheet

cf00a4b

Set if shouldShowRatingsPrompt after successful clip

ed3628c

Add more temp code for easier development

5fad9c0

Increment numSuccessfulClips in storage

b0747fe

Bypass storage for cached value of shouldShowRatingsPrompt if exists

7770c0f

Set lastBadRatingDate when appropriate

5afe600

Never show ratings prompt again after two bad ratings

50a2730

Add magic min successful clips # for ratings prompt to constants; min…

c4a6a90

…or renaming

DialogPanel now accepts a div ID; add more ratings styling

b880845

Do not prompt ratings after: second bad rating, any good rating

21be112

Gate the RATE stage on whether there's a rating url in settings for t…

775b8f3

…he client

Add global ratings prompt enabled gate per client

e9165b8

Basic ratings prompt animations (that sometimes look glitchy)

d425442

Move set of lastBadRatingDate storage value

23dd3f1

Refactor out some ratings prompt logic from SuccessPanel to RatingsHe…

4b1c7d0

…lper

Add strings to localization file

7cba727

Add button ids for ratings prompt

624fc8f

Hook up feedback link when it exists in settings

71000ed

Cleanup/add more to-dos

45c0c02

Move ratingsPromptStage from ClipperState to SuccessPanelState

859f80b

Track lastBadRatingVersion; add to bad rating delay logic

9a9d29a

Move generateFeedbackUrl test from footer_tests to utils_tests

b8af5f8

Units tests and related changes

a250d30

Update design to latest redlines

ec147e6

Clean up more to-dos

4e3e261

Log ShouldShowRatingsPrompt event

add64bd

Final cleanup before PR; remove dev-only changes

58f2504

VishaalK reviewed Sep 13, 2016
View reviewed changes

iconix added 7 commits September 15, 2016 12:41

Create a ratings panel

6784189

Make ShouldShowRatingsPrompt a first-class logging property

a037fe0

Set SV for numSuccessfulClips in order to unchain some promises in cl…

71d5013

…ipper.tsx; increment as part of SaveToOneNote.startClip

Some renaming and repurposing; tests are even more broken now, yay

d11f9b1

Allow for storage value caching, and use this feature-wide; fix tests…

07f4465

…; fix small bugs

ratingsPanel_tests; more cleanup

24a12b0

Use version.ts; other minor improvements from PR comments

ac0a7dd

iconix mentioned this pull request Sep 19, 2016

Implement safer solution for testing custom settings #35

Open

VishaalK approved these changes Sep 19, 2016

View reviewed changes

Minor bug fixes

b2d3eb9

VishaalK approved these changes Sep 19, 2016

View reviewed changes

Revert change to .height-animator that made it not expand down enough…

a15ea83

… on the SignInPanel; fix what that broke on RatingsPanel

iconix mentioned this pull request Sep 20, 2016

Unify mockStorage objects in tests #41

Closed

Resolve more minor TODOs

9c5c4e7

iconix mentioned this pull request Sep 20, 2016

Make async/await available to the code base #43

Open

iconix added 4 commits September 20, 2016 17:10

Merge with master

d7475be

Minor RatingsHelper cleanup

db32a60

Implement RemoteStorage.getValues and use in Clipper.preCacheStoredVa…

bf54d50

…lues

Simplify setLastBadRating(...) -> badRatingAlreadyOccurred()

35c1989

iconix force-pushed the feature/ratings-prompt branch from 7bbbced to fa09552 Compare September 26, 2016 21:34

Fix disabled clip button now that it has a border

d0f287d

iconix force-pushed the feature/ratings-prompt branch from fa09552 to d0f287d Compare September 26, 2016 21:36

iconix and others added 3 commits September 28, 2016 17:25

Add showRatingsPrompt to ClipperState reset func

e8b5a81

Merge with master

faa91b7

Merge branch 'master' of github.com:OneNoteDev/WebClipper into featur…

5e719e1

…e/ratings-prompt

leakymemory merged commit 092bfa9 into master Sep 29, 2016

leakymemory deleted the feature/ratings-prompt branch September 29, 2016 21:39

iconix mentioned this pull request Oct 3, 2016

[Bug] Ratings Prompt: Animations #76

Merged

iconix mentioned this pull request Oct 11, 2016

Implement ratings prompt animations as designed #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Ratings prompt #22

[Feature] Ratings prompt #22

iconix commented Sep 12, 2016 •

edited

Loading

VishaalK Sep 13, 2016

iconix Sep 14, 2016 •

edited

Loading

VishaalK Sep 14, 2016 •

edited

Loading

iconix Sep 15, 2016 •

edited

Loading

VishaalK Sep 19, 2016 •

edited

Loading

iconix Sep 19, 2016 •

edited

Loading

VishaalK Sep 19, 2016 •

edited

Loading

iconix Sep 20, 2016

VishaalK Sep 19, 2016

iconix Sep 20, 2016

VishaalK Sep 19, 2016

iconix Sep 20, 2016

VishaalK Sep 19, 2016

iconix Sep 20, 2016

VishaalK Sep 19, 2016

iconix Sep 20, 2016 •

edited

Loading

VishaalK Sep 19, 2016

leakymemory Sep 20, 2016

mttwc Sep 20, 2016

iconix Sep 20, 2016 •

edited

Loading

iconix commented Sep 19, 2016 •

edited

Loading

[Feature] Ratings prompt #22

[Feature] Ratings prompt #22

Conversation

iconix commented Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

iconix Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

VishaalK Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

iconix Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

VishaalK Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

iconix Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

VishaalK Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iconix Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iconix Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

iconix commented Sep 19, 2016 • edited Loading

iconix commented Sep 12, 2016 •

edited

Loading

iconix Sep 14, 2016 •

edited

Loading

VishaalK Sep 14, 2016 •

edited

Loading

iconix Sep 15, 2016 •

edited

Loading

VishaalK Sep 19, 2016 •

edited

Loading

iconix Sep 19, 2016 •

edited

Loading

VishaalK Sep 19, 2016 •

edited

Loading

iconix Sep 20, 2016 •

edited

Loading

iconix Sep 20, 2016 •

edited

Loading

iconix commented Sep 19, 2016 •

edited

Loading