Bug Fix: Renaming already uploaded profile #2810 #2843

GrooChu · 2020-10-06T09:15:05Z

PR for issue #2809
update the action changeProfileName to retrieve the profile and update the profile name. I have the tested the changes and it works fine without any issue.

Fixes #2809

codecov · 2020-10-07T05:48:18Z

Codecov Report

Merging #2843 (51239bd) into main (2695e41) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2843   +/-   ##
=======================================
  Coverage   88.32%   88.33%           
=======================================
  Files         241      241           
  Lines       19126    19138   +12     
  Branches     4893     4896    +3     
=======================================
+ Hits        16893    16905   +12     
  Misses       2070     2070           
  Partials      163      163

Impacted Files	Coverage Δ
src/actions/profile-view.js	`85.08% <100.00%> (+0.15%)`	⬆️
src/app-logic/published-profiles-store.js	`88.63% <100.00%> (+2.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2695e41...51239bd. Read the comment docs.

julienw

Thanks, this looks quite solid to me!
I just left 2 or 3 suggestions, but I think this will be good to go after this. Thanks again!

julienw · 2020-10-09T14:46:56Z

src/actions/profile-view.js

+      const hash = getHash(getState());
+      const storedProfile = await retrieveProfileData(hash);
+      if (storedProfile && storedProfile.name !== profileName) {
+        (storedProfile: any).name = profileName;


Instead of using any it's a good idea to work in a "immutable" way. In this case this is as easy as :

const newProfileData = { ...storedProfile, name: profileName, };

Fixed it based on your comments. Thanks

julienw · 2020-10-09T14:49:38Z

src/test/components/UploadedRecordingsHome.test.js

+    const newUrlState = stateFromLocation({
+      pathname: `/public/FAKE_1234/marker-chart/`,
+      search: '',
+      hash: '',
+    });
+    store.dispatch({
+      type: 'UPDATE_URL_STATE',
+      newUrlState,
+    });


I'm just wondering why that part is needed :-) can you please explain this? If that's not needed just remove it please!

Suggestion: maybe that's because you're testing UploadedRecordingsHome, maybe you should test ListOfPublishedProfiles instead?

Since we are using const hash = getHash(getState()); in the changeProfileName action, It is giving hash as null untill we do above UPDATE_URL_STATE action

julienw · 2020-10-09T14:50:05Z

src/test/components/UploadedRecordingsHome.test.js

+  it('can rename stored profiles', async () => {
+    // Store a single profile that we will rename. The data inside is mostly
+    // arbirtrary except the "name" field.
+    // mockDate('4 Jul 2020 15:00');


I'd suggest to keep the mock, so that we don't have subtle problems later.

Still there^

julienw · 2020-10-09T16:33:00Z

src/test/components/UploadedRecordingsHome.test.js

+    await storeProfileData({
+      profileToken: 'FAKE_1234',
+      jwtToken: null,
+      publishedDate: new Date('4 Jul 2020 13:00'), // This is the future!
+      name: 'Initial Profile Name',
+      preset: null,
+      originHostname: 'https://mozilla.org',
+      meta: {
+        product: 'Firefox',
+        platform: 'Macintosh',
+        toolkit: 'cocoa',
+        misc: 'rv:62.0',
+        oscpu: 'Intel Mac OS X 10.12',
+      },
+      urlPath: '/public/FAKE_1234/marker-chart/',
+      publishedRange: { start: 2000, end: 40000 },
+    });


If you move this test to ListOfPublishedProfiles.test.js you can reuse listOfProfileInformations, for example:

const profileData = { ...listOfProfileInformations[0], name: 'XXXX', };

I still think this should be moved to ListOfPublishedProfiles.test.js for simplicity...

julienw

Thanks for your changes and explanations.

Here are a few more comments, mostly moving things around to make it more understandable. But I think we're getting really close. Thanks again!

julienw · 2020-10-16T13:35:12Z

src/actions/profile-view.js

@@ -1357,12 +1357,15 @@ export function changeProfileName(
  profileName: string | null
 ): ThunkAction<Promise<void>> {
  return async (dispatch, getState) => {
-    if (window.indexedDB) {
+    if (window.indexedDB && profileName) {


If profileName is null or empty, we should still store a blank string, otherwise we won't be able to remove it.

julienw · 2020-10-16T13:36:12Z

src/test/components/UploadedRecordingsHome.test.js

+  it('can rename stored profiles', async () => {
+    // Store a single profile that we will rename. The data inside is mostly
+    // arbirtrary except the "name" field.
+    // mockDate('4 Jul 2020 15:00');


Still there^

julienw · 2020-10-16T13:36:16Z

src/test/components/UploadedRecordingsHome.test.js

+    await storeProfileData({
+      profileToken: 'FAKE_1234',
+      jwtToken: null,
+      publishedDate: new Date('4 Jul 2020 13:00'), // This is the future!
+      name: 'Initial Profile Name',
+      preset: null,
+      originHostname: 'https://mozilla.org',
+      meta: {
+        product: 'Firefox',
+        platform: 'Macintosh',
+        toolkit: 'cocoa',
+        misc: 'rv:62.0',
+        oscpu: 'Intel Mac OS X 10.12',
+      },
+      urlPath: '/public/FAKE_1234/marker-chart/',
+      publishedRange: { start: 2000, end: 40000 },
+    });


I still think this should be moved to ListOfPublishedProfiles.test.js for simplicity...

julienw · 2020-10-16T13:46:55Z

src/test/components/UploadedRecordingsHome.test.js

+    const newUrlState = stateFromLocation({
+      pathname: `/public/FAKE_1234/marker-chart/`,
+      search: '',
+      hash: '',
+    });
+    store.dispatch(updateUrlState(newUrlState));
+
+    await store.dispatch(changeProfileName('My Profile Name'));


Thanks for the explanation!

To have a better test and easier to maintain, I believe we should have 2 different stores (to simulate 2 different pages):

store 1 for the list of recordings => a blankStore is fine for that one.

store 2 for the profile viewer => you can use blankStore + dispatch the url state change for this one like you already did, and dispatch the profile name change on this store.

julienw · 2020-10-28T16:33:47Z

hey @GrooChu , are you still interested in finishing this patch? thanks :-)

julienw · 2020-11-09T16:36:24Z

Hey @canova, I've moved forward and fixed my last issues in this patch. Would you be kind enough to look at it?

Here is the STR I was looking at:

Publish a profile, any profile, from the deploy preview, for example this profile
Open the uploaded recordings view
Open the profile in the list in a new tab
Edit the profile's name from this new tab
Come back to the previous tab => the name should be changed in the list
Come back to the profile viewer tab and clear the name
Come back to the previous tab => the name should be back to default

Thanks!

update 1: I realized I missed something: after renaming, when opening the link, we still have the old profile name.
update 2: This is now ready again. I added some more commits to fix this, and also adding analytics for this feature. It could be a good idea to look at individual commits before looking at the big picture.

Fixes firefox-devtools#2809

…use 2 different stores

…profiles-store

I implemented the changes myself and waiting for a review from another team member.

canova

Looks pretty good to me, thanks for working on it!

canova · 2020-11-16T12:55:44Z

src/app-logic/published-profiles-store.js

+ * This stores some profile data. The profileToken property is the primary key,
+ * so this also updates any profile data already there with the same
+ * profileToken information.
+ */


Thanks for adding comments to the functions! It helped me to figure out about updating profile data with same profileToken information.

julienw · 2020-12-03T11:03:08Z

update 1: I realized I missed something: after renaming, when opening the link, we still have the old profile name.

~~I'm confused, this seems to still be a problem...~~
ah nope, I had an old entry from before the fix.

GrooChu force-pushed the profile-rename branch from 5326a5f to 5042e9e Compare October 7, 2020 05:39

GrooChu mentioned this pull request Oct 8, 2020

#2840 | Make profile name button look like other menu button #2846

Merged

julienw requested changes Oct 9, 2020

View reviewed changes

julienw reviewed Oct 9, 2020

View reviewed changes

julienw previously requested changes Oct 16, 2020

View reviewed changes

julienw self-assigned this Nov 9, 2020

julienw force-pushed the profile-rename branch from d3a1f28 to 83d09b9 Compare November 9, 2020 16:24

julienw requested review from canova and removed request for canova November 9, 2020 16:38

julienw force-pushed the profile-rename branch from 83d09b9 to 8ef4c06 Compare November 10, 2020 13:58

Sabesh Kumar Rajendran and others added 6 commits November 10, 2020 15:00

Change the stored profile data when changing the profile name in the UI

d468a93

Fixes firefox-devtools#2809

Review comment: move the test to ListOfPublishedProfiles.test.js and …

9cdf394

…use 2 different stores

Make it possible to unset the name of a profile

5d36038

Move the logic to handle the stored profile name change to published-…

11e950f

…profiles-store

When changing the profile name, change the urlPath as well

e2dc0f2

Add an analytics ping for the profile name change

7a49a97

julienw force-pushed the profile-rename branch from 8ef4c06 to 7a49a97 Compare November 10, 2020 14:09

julienw requested a review from canova November 10, 2020 14:12

canova approved these changes Nov 16, 2020

View reviewed changes

Merge branch 'main' into profile-rename

992e320

Merge branch 'main' into profile-rename

51239bd

julienw merged commit 767aa2f into firefox-devtools:main Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fix: Renaming already uploaded profile #2810 #2843

Bug Fix: Renaming already uploaded profile #2810 #2843

GrooChu commented Oct 6, 2020

codecov bot commented Oct 7, 2020 •

edited

Loading

julienw left a comment

julienw Oct 9, 2020

GrooChu Oct 15, 2020

julienw Oct 9, 2020

julienw Oct 9, 2020

GrooChu Oct 15, 2020

julienw Oct 9, 2020

julienw Oct 16, 2020

julienw Oct 9, 2020 •

edited

Loading

julienw Oct 16, 2020

julienw left a comment

julienw Oct 16, 2020

julienw Oct 16, 2020

julienw Oct 16, 2020

julienw Oct 16, 2020

julienw commented Oct 28, 2020

julienw commented Nov 9, 2020 •

edited

Loading

canova left a comment

canova Nov 16, 2020

julienw commented Dec 3, 2020 •

edited

Loading

Bug Fix: Renaming already uploaded profile #2810 #2843

Bug Fix: Renaming already uploaded profile #2810 #2843

Conversation

GrooChu commented Oct 6, 2020

codecov bot commented Oct 7, 2020 • edited Loading

Codecov Report

julienw left a 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

julienw Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw left a 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

julienw commented Oct 28, 2020

julienw commented Nov 9, 2020 • edited Loading

canova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw commented Dec 3, 2020 • edited Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

julienw Oct 9, 2020 •

edited

Loading

julienw commented Nov 9, 2020 •

edited

Loading

julienw commented Dec 3, 2020 •

edited

Loading