Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app): display correct creation method for PD protocol exported i… #17724

Open
wants to merge 4 commits into
base: edge
Choose a base branch
from

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Mar 11, 2025

…n py

closes AUTH-1525

Overview

If a protocol was made in PD and downloaded in py, we want the creation method to reflect the PD schema version

Screenshot 2025-03-11 at 14 56 03

Test Plan and Hands on Testing

Review the code and test uploading to the app. look at the protocol details's creation method. here is a protocol you can test with:

ot2AllModules.py.zip

Changelog

  • add logic for reading the protocolDesigner metadata
  • add test case

Risk assessment

low, shouldn't encounter this in prod

@jerader jerader requested a review from a team as a code owner March 11, 2025 18:55
@jerader jerader requested review from koji, ddcc4 and ncdiehl11 and removed request for a team March 11, 2025 18:55
})
if ('protocolDesigner' in metadata) {
return t('protocol_designer_version', {
version: parseInt(metadata.protocolDesigner).toFixed(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

How does parseInt() work here? If the string is

"protocolDesigner": "8.4.3",

do you want this to return 8.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

8.4.3 -> 8, then the toFixed() goes to 8.0. We want to display the schema version to match a PD protocol exported in json

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! That was what was mentioned on the call about the PD version being the schema version.

Copy link

codecov bot commented Mar 11, 2025

Codecov Report

Attention: Patch coverage is 61.11111% with 7 lines in your changes missing coverage. Please review.

Project coverage is 62.47%. Comparing base (b428d0a) to head (54ed08b).
Report is 1 commits behind head on edge.

Files with missing lines Patch % Lines
...pp/src/organisms/Desktop/ProtocolDetails/index.tsx 61.11% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #17724   +/-   ##
=======================================
  Coverage   62.47%   62.47%           
=======================================
  Files        2792     2792           
  Lines      215277   215289   +12     
  Branches    18414    18416    +2     
=======================================
+ Hits       134498   134511   +13     
+ Misses      80594    80593    -1     
  Partials      185      185           
Flag Coverage Δ
app 46.31% <61.11%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pp/src/organisms/Desktop/ProtocolDetails/index.tsx 71.61% <61.11%> (+0.65%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

createdAt,
metadata: {
...mockMostRecentAnalysis.metadata,
protocolDesigner: '8.0.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this 8.12.34 to demonstrate that we extract it as 8.0 no matter what the minor version is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, i'll do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants