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(IT Wallet): [SIW-1638] Handle unknown credential status #6576

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

gispada
Copy link
Collaborator

@gispada gispada commented Dec 20, 2024

Short description

This PR handles unexpected failures of the status attestation call. When that happens (for instance because of network problems), it is not possibile to determine the status of a credential. This "unknown" status is reflected in the UI and the credential detail cannot be accessed.

List of changes proposed in this pull request

  • Added new card assets for the "Not available" status
  • Explicitly return unknown in getCredentialStatus (previously an unknown status attestation would return valid)
  • Modified ItwCredentialCard to display the greyed-out card
  • Added the new status in the cards DS section

How to test

The easiest way to test this PR is by mocking the result of the /status endpoint so that it returns 500.

Copy link
Contributor

github-actions bot commented Dec 20, 2024

Jira Pull Request Link

This Pull Request refers to the following Jira issue SIW-1638

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 49.31%. Comparing base (80d7bd5) to head (2f0a36e).

Files with missing lines Patch % Lines
...ponents/ItwPresentationCredentialUnknownStatus.tsx 14.28% 6 Missing ⚠️
.../screens/ItwPresentationCredentialDetailScreen.tsx 0.00% 2 Missing ⚠️
...allet/common/components/ItwDigitalVersionBadge.tsx 92.85% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6576      +/-   ##
==========================================
+ Coverage   49.29%   49.31%   +0.01%     
==========================================
  Files        1555     1559       +4     
  Lines       32122    32151      +29     
  Branches     7267     7274       +7     
==========================================
+ Hits        15834    15854      +20     
- Misses      16250    16259       +9     
  Partials       38       38              
Files with missing lines Coverage Δ
img/features/itWallet/cards/dc_na.png 100.00% <ø> (ø)
img/features/itWallet/cards/mdl_na.png 100.00% <ø> (ø)
img/features/itWallet/cards/ts_na.png 100.00% <ø> (ø)
ts/features/design-system/core/DSCards.tsx 53.33% <ø> (ø)
ts/features/itwallet/analytics/index.ts 32.95% <ø> (ø)
...s/itwallet/common/components/ItwCredentialCard.tsx 100.00% <100.00%> (ø)
.../itwallet/common/utils/itwCredentialStatusUtils.ts 77.14% <100.00%> (+1.38%) ⬆️
...atures/itwallet/common/utils/itwCredentialUtils.ts 88.88% <ø> (ø)
...allet/common/components/ItwDigitalVersionBadge.tsx 95.00% <92.85%> (-5.00%) ⬇️
.../screens/ItwPresentationCredentialDetailScreen.tsx 9.67% <0.00%> (-0.67%) ⬇️
... and 1 more

Continue to review full report in Codecov by Sentry.

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

@gispada gispada marked this pull request as ready for review December 20, 2024 10:45
Comment on lines 44 to 56
const colorProps = useMemo(() => {
const baseColorProps = mapCredentialTypes[credentialType];
if (!baseColorProps) {
return;
}
if (colorScheme === "greyscale") {
return {
foreground: Color(baseColorProps.foreground).grayscale().hex(),
background: Color(baseColorProps.background).grayscale().hex()
};
}
};
return baseColorProps;
}, [credentialType, colorScheme]);
Copy link
Contributor

@mastro993 mastro993 Jan 3, 2025

Choose a reason for hiding this comment

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

Just a little note: this useMemo is a bit redundant and does not provide any benefit. colorProps depends on the same props as the component, it will be recalculated every time the component re-renders anyway.

Keeping the memoization has negligible overhead, tho.
Feel free to do what you think is most appropriate!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, I have removed two useMemo that were dependent on all the component props (2f0a36e).

Copy link
Contributor

@mastro993 mastro993 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

2 participants