[BIOMAGE-1835] Implement samples PATCH for api v2 #686
Conversation
cosa65
changed the title
…rror if it is being used incorrectly (so when it is used to update metadataTracks)
cosa65
force-pushed
the
1835-samples-patch-api-v2
branch
from
2beac15 to
cba5a7f
Compare
📦 Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action 🤖
|
| Page | Size (compressed) |
|---|---|
global |
522.25 KB (🟡 +544 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!
Eleven Pages Changed Size
The following pages changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/data-management |
296.02 KB (🟡 +531 B) |
818.27 KB |
/experiments/[experimentId]/data-exploration |
541.9 KB (🟡 +100 B) |
1.04 MB |
/experiments/[experimentId]/data-processing |
503.57 KB (🟡 +616 B) |
1 MB |
/experiments/[experimentId]/plots-and-tables/dot-plot |
420.34 KB (🟢 -228 B) |
942.59 KB |
/experiments/[experimentId]/plots-and-tables/embedding-categorical |
413.68 KB (🟢 -637 B) |
935.93 KB |
/experiments/[experimentId]/plots-and-tables/embedding-continuous |
414.47 KB (🟢 -206 B) |
936.72 KB |
/experiments/[experimentId]/plots-and-tables/frequency |
417.89 KB (🟢 -246 B) |
940.14 KB |
/experiments/[experimentId]/plots-and-tables/heatmap |
490.66 KB (🟢 -213 B) |
1012.91 KB |
/experiments/[experimentId]/plots-and-tables/marker-heatmap |
493.11 KB (🟢 -216 B) |
1015.36 KB |
/experiments/[experimentId]/plots-and-tables/violin |
417.21 KB (🟢 -636 B) |
939.46 KB |
/experiments/[experimentId]/plots-and-tables/volcano |
420.26 KB (🟢 -621 B) |
942.51 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.
Codecov Report
@@ Coverage Diff @@
## master #686 +/- ##
==========================================
+ Coverage 81.84% 81.90% +0.05%
==========================================
Files 466 466
Lines 7895 7912 +17
Branches 1520 1523 +3
==========================================
+ Hits 6462 6480 +18
+ Misses 1376 1375 -1
Partials 57 57
Continue to review full report at Codecov.
|
|
|
||
| enableFetchMocks(); | ||
| fetchMock.resetMocks(); | ||
| fetchMock.doMock(); |
There was a problem hiding this comment.
are these all required? just mentioning because I've seen some tests where many of these are just copy pasted but aren't needed. What does doMock do by the way?
There was a problem hiding this comment.
in this case yes, we are mocking the fetch for api v2 so we can say that the request succeeeded
|
|
||
| const updateSample = ( | ||
| sampleUuid, | ||
| diff, | ||
| ) => async (dispatch, getState) => { | ||
| const sample = _.cloneDeep(getState().samples[sampleUuid]); | ||
|
|
||
| // eslint-disable-next-line no-param-reassign | ||
| diff.lastModified = moment().toISOString(); | ||
| try { |
There was a problem hiding this comment.
try/catch best practices is to make them as short as possible. I would wrap only the actions dispatch or something like that.
There was a problem hiding this comment.
I wrapped all the things that need to not happen if an error takes place because the catch doesn't rethrow so it continues executing after getting out of it.
There was a problem hiding this comment.
I think the PR structure needs more work. Having to support v1 and v2 will make the code messier so I think we should take extra care to make it as separate and clear as possible. If you think that's not possible or requires too much effort I think we should seriously consider other migration options because this is very error prone.
Description
Details
URL to issue
https://biomage.atlassian.net/browse/BIOMAGE-1835
https://ui-martinfosco-ui686-api334.scp-staging.biomage.net/
Link to staging deployment URL (or set N/A)
N/A
Links to any PRs or resources related to this PR
hms-dbmi-cellenics/api#334
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
Manual/unit testing
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.
Documentation updates
Optional