[HOTFIX] Rename metadata tracks #776

cosa65 · 2022-07-19T20:03:23Z

Description

Details

URL to issue

N/A

Link to staging deployment URL (or set N/A)

https://ui-martinfosco-ui776.scp-staging.biomage.net/

Links to any PRs or resources related to this PR

Integration test branch

master

Merge checklist

Your changes will be ready for merging after all of the steps below have been completed.

Code updates

Have best practices and ongoing refactors being observed in this PR

Migrated any selector / reducer used to the new format.

Manual/unit testing

Tested changes using InfraMock locally or no tests required for change, e.g. Kubernetes chart updates.
Validated that current unit tests for code work as expected and are sufficient for code coverage or no unit tests required for change, e.g. documentation update.
Unit tests written or no unit tests required for change, e.g. documentation update.

Integration testing

You must check the box below to run integration tests on the latest commit on your PR branch.
Integration tests have to pass before the PR can be merged. Without checking the box, your PR
will not pass the required status checks for merging.

Started end-to-end tests on the latest commit.

Documentation updates

Relevant Github READMEs updated or no GitHub README updates required.
Relevant Wiki pages created/updated or no Wiki updates required.

Optional

Staging environment is unstaged before merging.
Photo of a cute animal attached to this PR.

codecov · 2022-07-19T20:22:54Z

Codecov Report

Merging #776 (1a387c4) into master (f62b605) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #776      +/-   ##
==========================================
+ Coverage   83.69%   83.74%   +0.04%     
==========================================
  Files         467      468       +1     
  Lines        7910     7922      +12     
  Branches     1525     1526       +1     
==========================================
+ Hits         6620     6634      +14     
+ Misses       1238     1236       -2     
  Partials       52       52

Impacted Files	Coverage Δ
...omponents/data-management/LaunchAnalysisButton.jsx	`98.43% <ø> (ø)`
src/components/data-management/MetadataColumn.jsx	`63.15% <ø> (ø)`
src/components/data-management/SamplesTable.jsx	`85.57% <100.00%> (+0.42%)`	⬆️
src/redux/actionTypes/experiments.js	`100.00% <100.00%> (ø)`
...c/redux/actions/experiments/renameMetadataTrack.js	`92.85% <100.00%> (ø)`
.../reducers/experiments/experimentsMetadataDelete.js	`100.00% <100.00%> (+25.00%)`	⬆️
.../reducers/experiments/experimentsMetadataRename.js	`100.00% <100.00%> (ø)`
...edux/reducers/samples/experimentsMetadataRename.js	`100.00% <100.00%> (ø)`

github-actions · 2022-07-19T20:24:19Z

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

⚠️ Global Bundle Size Increased

Page	Size (compressed)
`global`	`521.92 KB` (🟡 +53 B)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

One Page Changed Size

The following page changed size from the code in this PR compared to its base branch:

Page	Size (compressed)	First Load
`/data-management`	`246.43 KB` (🟡 +15 B)	768.35 KB

Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

…e the position of the renamed track

… be loaded before initializin the metadata columns

cosa65 · 2022-07-19T22:15:52Z

src/components/data-management/SamplesTable.jsx

+    const samplesLoaded = activeExperiment?.sampleIds.every((sampleId) => samples[sampleId]);
+
+    if (activeExperiment?.sampleIds.length > 0 && samplesLoaded) {
      // if there are samples - build the table columns
-      setSampleNames(new Set(activeExperiment.sampleIds.map((id) => samples[id]?.name.trim())));
+
+      const sanitizedSampleNames = new Set(
+        activeExperiment.sampleIds.map((id) => samples[id]?.name.trim()),
+      );
+
+      setSampleNames(sanitizedSampleNames);


We don't load the samples immediately as soon as we start, so I added some checks that the samples are loaded first before going through with rendering

cosa65 · 2022-07-19T22:20:44Z

src/redux/actions/experiments/renameMetadataTrack.js

-
-    experiment.sampleIds.forEach((sampleUuid) => {
-      dispatch({
-        type: SAMPLES_UPDATE,
-        payload: {
-          sampleUuid,
-          sample: { metadata: { [newMetadataKey]: samples[sampleUuid].metadata[oldMetadataKey] } },
-        },
-      });
-
-      dispatch({
-        type: SAMPLES_METADATA_DELETE,
-        payload: {
-          metadataKey: oldMetadataKey,
-          sampleUuid,
-        },
-      });
-    });


I think the real issue was here (the rest of the changes was me refactoring), if a user adds a space after the metadata name it would not notice that the name had changed (trailing spaces are removed) so what happened here was the following:

User has a metadata track "Track 1"

User sets new name "Track 1 ":

The trailing space is removed so this means that:
oldMetadataKey = 'Track 1'
newMetadataKey = 'Track 1'

SAMPLES_UPDATE: Sets sample.metadata['Track 1'] = sample.metadata['Track 1']

SAMPLES_METADATA_DELETE: Deletes sample.metadata['Track 1']

So we ended up with a sample with an undefined value for the renamed metadata track, this would cause crashes when there was any further interaction

This is where the fix happens

really interesting and obscure bug

cosa65 · 2022-07-19T22:23:02Z

src/redux/reducers/samples/experimentsMetadataRename.js

+  sampleIdsFromExperiment.forEach((sampleId) => {
+    const value = draft[sampleId].metadata[oldKey];
+
+    delete draft[sampleId].metadata[oldKey];
+
+    draft[sampleId].metadata[newKey] = value;


The fix for the bug is here:

We store the value in a temporary variable, delete the oldKey and then set the newKey, this ensures it will work even if oldKey === newKey

why not just if "oldKey" == "newKey", do not update?

it seemed safer to implement a simple solution that covers all cases rather than having a guard for the case where it breaks and then have the rest of the solution

cosa65 · 2022-07-19T22:26:06Z

src/redux/reducers/experiments/experimentsMetadataRename.js

+const experimentsMetadataRename = (state, action) => {
+  const { oldKey, newKey, experimentId } = action.payload;
+
+  const { metadataKeys } = state[experimentId];
+  const newMetadataKeys = metadataKeys.map((metadataKey) => (
+    metadataKey === oldKey ? newKey : metadataKey
+  ));
+
+  return {
+    ...state,
+    [experimentId]: {
+      ...state[experimentId],
+      metadataKeys: newMetadataKeys,
+    },
+  };
+};
+
+export default experimentsMetadataRename;


This is just a rename of experimentsMetadataUpdate, one extra thing I did here is deal with the annoying thing where if we renamed a metadata track key, it would push the renamed key to the end.

Look at src/redux/reducers/experiments/experimentsMetadataUpdate.js to see what was changed

.

Remove metadata delete action dispatch

89b95ca

cosa65 changed the title ~~[HOTFIX] Remove metadata delete action dispatch~~ [HOTFIX] Rename metadata tracks Jul 19, 2022

cosa65 marked this pull request as ready for review July 19, 2022 20:03

ivababukova previously approved these changes Jul 19, 2022

View reviewed changes

cosa65 added 4 commits July 19, 2022 18:48

Move metadata track key rename to reducer

5e717a6

Make experimentsMetadataRename reducer in experiments slice not chang…

4420bb4

…e the position of the renamed track

Add some checks to wait for the sampleIds of the active experiment to…

f189152

… be loaded before initializin the metadata columns

Fix so it works with newKey === oldKey

f898fe7

cosa65 requested a review from ivababukova July 19, 2022 22:09

cosa65 commented Jul 19, 2022

View reviewed changes

cosa65 added 4 commits July 19, 2022 19:39

Update test

8036371

Fix experimentsReducer

613978c

Rename some tests

6fd610b

Fix test

b6f3181

ivababukova approved these changes Jul 20, 2022

View reviewed changes

cosa65 added 2 commits July 20, 2022 09:09

Add test

44c9121

Update snapshot

1a387c4

cosa65 merged commit dcfcb2c into master Jul 20, 2022

cosa65 deleted the hotfix-rename-metadata-tracks branch July 20, 2022 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOTFIX] Rename metadata tracks #776

[HOTFIX] Rename metadata tracks #776

cosa65 commented Jul 19, 2022 •

edited by ivababukova

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading

github-actions bot commented Jul 19, 2022 •

edited

Loading

cosa65 Jul 19, 2022

cosa65 Jul 19, 2022 •

edited

Loading

cosa65 Jul 19, 2022

ivababukova Jul 20, 2022

cosa65 Jul 19, 2022 •

edited

Loading

aerlaut Jul 20, 2022

cosa65 Jul 20, 2022 •

edited

Loading

cosa65 Jul 19, 2022

[HOTFIX] Rename metadata tracks #776

[HOTFIX] Rename metadata tracks #776

Conversation

cosa65 commented Jul 19, 2022 • edited by ivababukova Loading

Description

Details

URL to issue

Link to staging deployment URL (or set N/A)

Links to any PRs or resources related to this PR

Integration test branch

Merge checklist

Code updates

Manual/unit testing

Integration testing

Documentation updates

Optional

codecov bot commented Jul 19, 2022 • edited Loading

Codecov Report

github-actions bot commented Jul 19, 2022 • edited Loading

📦 Next.js Bundle Analysis

⚠️ Global Bundle Size Increased

One Page Changed Size

cosa65 Jul 19, 2022

Choose a reason for hiding this comment

cosa65 Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

cosa65 Jul 19, 2022

Choose a reason for hiding this comment

ivababukova Jul 20, 2022

Choose a reason for hiding this comment

cosa65 Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

aerlaut Jul 20, 2022

Choose a reason for hiding this comment

cosa65 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

cosa65 Jul 19, 2022

Choose a reason for hiding this comment

cosa65 commented Jul 19, 2022 •

edited by ivababukova

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading

github-actions bot commented Jul 19, 2022 •

edited

Loading

cosa65 Jul 19, 2022 •

edited

Loading

cosa65 Jul 19, 2022 •

edited

Loading

cosa65 Jul 20, 2022 •

edited

Loading