Testpilot ga #268

abhinadduri · 2017-07-20T22:18:07Z

Finished adding in all metrics.

One issue is that a user can click on Try Firefox Send without completing a download, something that we don't account for in our metrics guidelines for the restarted event.

pdehaan · 2017-07-20T22:33:51Z

frontend/src/upload.js

+ for (let i = 0; i < localStorage.length; i++) {
+ const id = localStorage.key(i);
+ //check if file exists before adding to list
+ checkExistence(id, true);


I'm a bit confused here... We're looping over localStorage and checking each key to see if the file exists. But now we're injecting more things into localStorage, such as referrer, etc.

Should we be filtering out ids against a regexp before calling setExistence()? Or should we possibly refactor the files into a specific key (such as localStorage.files which is an array/Object of all the non-expired files?

Further down in the code, checkExistence does a check to ignore the injected elements. We can move that up here if necessary.

... Because I notice we have code like this in ~4 places currently:

+ if (id !== 'totalUploads' && + id !== 'totalDownloads' && + id !== 'referrer') { + // ... + }

Maybe we need some utility function isFile(id) {} helper.

function isFile(id) { return !['referrer', 'totalDownloads', 'totalUploads'].contains(id); // optionally validate `id` against a RegExp. }

This would be a handy function to have! I'll go ahead and add it into utils.js.

pdehaan · 2017-07-20T22:45:17Z

frontend/src/upload.js

+ cm5: localStorage.getItem('totalUploads'),
+ cm6: unexpiredFiles,
+ cm7: totalDownloads,
+ cd1: event.type === 'drop' ? 'drop' : 'click',


Is there any way the event.type could not be "drop" or "click"?

The only way for the event.type to not be drop is if the user doesn't drag and drop the file. I'm not sure the onChange property of the input tag can be changed with anything other than a click or a drag for a file upload.

pdehaan · 2017-07-20T22:53:11Z

frontend/src/upload.js

 // delete file
 $popupText.find('.del-file').click(e => {
 FileSender.delete(file.fileId, file.deleteToken).then(() => {
 $(e.target).parents('tr').remove();
- localStorage.removeItem(file.fileId);
+ const timeToExpiry = 86400000 - (new Date().getTime() - file.creationDate.getTime());


Not sure if we want to convert the magic 86400000 number to a const. For example:

const timeToExpiry = ONE_DAY_IN_MS - Date.now() - file.creationDate.getTime();

Or better yet, extract into helper function since I think we do this in at least 2 places.

pdehaan · 2017-07-20T23:01:13Z

frontend/src/utils.js

+ case 'https://testpilot.firefox.com/privacy':
+ return 'privacy';
+ case 'https://testpilot.firefox.com/terms':
+ return 'terms';


Re: #260
Not sure what "privacy" and "terms" means here, or if we need to try clarifying to disambiguate between Send Privacy/Terms and TestPilot Privacy/Terms. Or not, I have no idea what I'm talking about.
/cc @chuckharmston @clouserw

They're intended to be the footer links. If there are additional link destinations I missed, feel free to disambiguate or add to the list as appropriate as long as you update the docs; I pieced this together from the mocks.

dannycoates · 2017-07-21T17:23:07Z

frontend/src/upload.js

+ $('#link').attr('disabled', false);
+ $copyBtn.attr('data-l10n-id', 'copyUrlFormButton');
+
+ if (localStorage.length === 0) {


with the new keys this check is no longer a valid way to determine whether to show the file list header

Fixed by adding in a new function in utils.js.

dannycoates · 2017-07-21T17:31:28Z

server/server.js

+ sizeInBytes: contentLength,
+ timeToExpiry: timeToExpiry,
+ trackerId: conf.analytics_id,
+ dsn: conf.sentry_id


trackerId and dsn should not need to be in this response anymore

dannycoates · 2017-07-21T17:35:47Z

frontend/src/upload.js

 fileSender.on('encrypting', isStillEncrypting => {
 // The file is being encrypted
 if (isStillEncrypting) {
 console.log('Encrypting');
 } else {
 console.log('Finished encrypting');
+ uploadStart = new Date().getTime();


Date.now() is the preferred way to do this

Changed all instances of new Date().getTime() to Date.now().

dannycoates · 2017-07-21T17:39:55Z

frontend/src/download.js

 const filename = $('#dl-filename').html();
+ const bytelength = $('#dl-bytelength').html();


should be .text() and converted to Number. same for timeToExpiry

Changed to const bytelength = Number($('#dl-bytelength').text());, and the same for timeToExpiry.

dannycoates · 2017-07-21T17:46:20Z

frontend/src/upload.js

+
+ let totalDownloads = 0;
+ if (localStorage.hasOwnProperty('totalDownloads')) {
+ totalDownloads = localStorage.getItem('totalDownloads');


we should be consistent with the type of totalDownloads and others. localStorage stores everything as a string so helper functions to convert for us will mean less repetition:

function getTotalDownloads() { return Number(localStorage.getItem('totalDownloads')) }

dannycoates · 2017-07-21T17:49:54Z

frontend/src/upload.js

@@ -200,7 +345,9 @@ $(document).ready(function() {
 populateFileList(localStorage.getItem(id));
 }
 } else if (xhr.status === 404) {
- localStorage.removeItem(id);
+ if (isFile(id)) {


we shouldn't even be calling checkExistence on non-file ids. move this up a level

checkExistence is no longer called if an id is not a file.

dannycoates · 2017-07-21T19:53:14Z

frontend/src/download.js

+ let totalUploads = 0;
+ if (storage.has('totalUploads')) {
+ totalUploads = storage.totalUploads;
+ }


we've got two blocks with this pattern. this is initialization code so i suggest moving them into the Storage constructor. Also check that they're necessary at all because I think the getters (as implemented) will return 0 when it's not already set.

Fixed by removing the check.

dannycoates · 2017-07-21T19:55:27Z

frontend/src/download.js

+ window.analytics
+ .sendEvent('recipient', 'download-stopped', {
+ cm1: bytelength,
+ cm5: totalUploads,


nit: lets use storage.totalUploads directly and remove the local var

Fixed in all cases.

pdehaan · 2017-07-21T20:06:46Z

frontend/src/storage.js

+ }
+ }
+ return length;
+ }


This looks mostly similar to get files() {...} above (with the obvious exception of returning a count instead of an array). Not sure if it's worth it just to return this.files.length, or if that is too 'expensive' due to the JSON.parse() calls.

This would probably be a lot easier if we just had a localStorage key named files which was a JSON encoded array/object of files, instead of storing everything at the root level and having to loop/filter. 🤷‍♀️

I avoided calling length because of the parsing calls. I'm not sure if it's worth it or not to just go with this.files.length.

we can revisit the internals of this later. for now im just glad it's factored into it's own module

pdehaan · 2017-07-21T20:08:36Z

views/download.handlebars

@@ -41,6 +43,6 @@
 <img src="/resources/illustration_expired.svg" id="expired-img" data-l10n-id="linkExpiredAlt"/>
 </div>
 <div class="expired-description" data-l10n-id="uploadPageExplainer"></div>
- <a class="send-new" data-l10n-id="sendYourFilesLink"></a>
+ <a class="send-new" id = "expired-send-new" data-l10n-id="sendYourFilesLink"></a>


Nit: remove spaces around =.

dannycoates · 2017-07-21T20:34:32Z

frontend/src/download.js

+
+if (!storage.has('totalDownloads')) {
+ storage.totalDownloads = 0;
+}


abhinadduri added 6 commits July 19, 2017 16:16

added analytics for copied, unsupported, and restarted metrics

744dbb3

Merge branch 'master' of github.com:mozilla/send into testpilotGA

76a6f02

finished metrics

9947777

Merge branch 'master' of github.com:mozilla/send into testpilotGA

e5d1e8f

updated one metric, cd4 added to upload-deleted

37bb6fd

linting

b9eb653

abhinadduri requested review from chuckharmston and dannycoates July 20, 2017 22:23

pdehaan reviewed Jul 20, 2017

View reviewed changes

fixed one bug, added some helper functions

cb0d69c

pdehaan mentioned this pull request Jul 21, 2017

Don't send analytics if DNT is enabled #196

Closed

dannycoates suggested changes Jul 21, 2017

View reviewed changes

abhinadduri added 2 commits July 21, 2017 12:44

refactored localStorage into storage module for frontend

d660eda

if checkexistence removes the last item in the list, call toggleHeader

14f3d83

dannycoates reviewed Jul 21, 2017

View reviewed changes

pdehaan reviewed Jul 21, 2017

View reviewed changes

final fixes

9585850

dannycoates reviewed Jul 21, 2017

View reviewed changes

frontend/src/download.js Outdated

if (!storage.has('totalDownloads')) {

storage.totalDownloads = 0;

}

Copy link

Contributor

dannycoates Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete me

abhinadduri added 2 commits July 21, 2017 13:36

merging master

09a6192

removing initialization on top in favor of default

b18bcd3

dannycoates approved these changes Jul 21, 2017

View reviewed changes

abhinadduri merged commit 7243a10 into master Jul 21, 2017

dannycoates deleted the testpilotGA branch July 31, 2017 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testpilot ga #268

Testpilot ga #268

abhinadduri commented Jul 20, 2017 •

edited

Loading

pdehaan Jul 20, 2017

abhinadduri Jul 20, 2017 •

edited

Loading

pdehaan Jul 20, 2017

abhinadduri Jul 20, 2017

pdehaan Jul 20, 2017

abhinadduri Jul 20, 2017

pdehaan Jul 20, 2017 •

edited

Loading

pdehaan Jul 20, 2017

chuckharmston Jul 20, 2017

dannycoates Jul 21, 2017

abhinadduri Jul 21, 2017

dannycoates Jul 21, 2017

abhinadduri Jul 21, 2017

dannycoates Jul 21, 2017

abhinadduri Jul 21, 2017

dannycoates Jul 21, 2017

abhinadduri Jul 21, 2017

dannycoates Jul 21, 2017

dannycoates Jul 21, 2017

abhinadduri Jul 21, 2017

dannycoates Jul 21, 2017

abhinadduri Jul 21, 2017

dannycoates Jul 21, 2017

abhinadduri Jul 21, 2017

pdehaan Jul 21, 2017

abhinadduri Jul 21, 2017

dannycoates Jul 21, 2017

pdehaan Jul 21, 2017

dannycoates Jul 21, 2017

		const filename = $('#dl-filename').html();
		const bytelength = $('#dl-bytelength').html();

Testpilot ga #268

Testpilot ga #268

Conversation

abhinadduri commented Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

abhinadduri Jul 20, 2017 • 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

pdehaan Jul 20, 2017 • 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

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

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

abhinadduri commented Jul 20, 2017 •

edited

Loading

abhinadduri Jul 20, 2017 •

edited

Loading

pdehaan Jul 20, 2017 •

edited

Loading