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

chore(IT Wallet): [SIW-1665] Hide eID card in wallet #6214

Merged
merged 29 commits into from
Oct 3, 2024

Conversation

gispada
Copy link
Collaborator

@gispada gispada commented Sep 26, 2024

Short description

This PR hides the eID card in the wallet screen. It also adds a bottom sheet to display information on the digital identity.

List of changes proposed in this pull request

  • Removed the eID card from the wallet screen
  • Added a banner to inform the user the wallet is active
  • Added a bottom sheet component to display the eID claims and the issuance date
  • Added expiration and issuedAt properties to StoredCredential type, with Redux Persist migration

How to test

If you have an existing wallet, check the following:

  • The migration worked, and every credential in the Redux Store now has issuedAt and expiration
  • The eID card is not visible in the wallet screen, only other credentials
  • The bottom sheet that appears pressing on "Cos'è?" properly displays the issuance date

Reset the wallet and get all credentials, then check the same points as above.

@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Sep 26, 2024

Affected stories

  • ⚙️ SIW-1665: [io-app] Sostituzione card eID con badge e bottom sheet nella sezione portafoglio

Generated by 🚫 dangerJS against ece3903

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 54.54545% with 30 lines in your changes missing coverage. Please review.

Project coverage is 47.15%. Comparing base (4f204b4) to head (ece3903).
Report is 533 commits behind head on master.

Files with missing lines Patch % Lines
...tures/itwallet/common/store/reducers/migrations.ts 12.00% 22 Missing ⚠️
...common/components/ItwEidInfoBottomSheetContent.tsx 69.23% 4 Missing ⚠️
.../credentials/saga/handleItwCredentialsStoreSaga.ts 0.00% 3 Missing ⚠️
...twallet/common/components/ItwWalletReadyBanner.tsx 88.88% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6214      +/-   ##
==========================================
- Coverage   48.42%   47.15%   -1.27%     
==========================================
  Files        1488     1781     +293     
  Lines       31617    35933    +4316     
  Branches     7669     8501     +832     
==========================================
+ Hits        15311    16945    +1634     
- Misses      16238    18930    +2692     
+ Partials       68       58      -10     
Files with missing lines Coverage Δ
ts/features/design-system/core/DSWallet.tsx 12.50% <ø> (-4.17%) ⬇️
.../itwallet/common/components/ItwDiscoveryBanner.tsx 84.61% <100.00%> (ø)
...s/features/itwallet/common/store/reducers/index.ts 100.00% <ø> (ø)
...twallet/common/utils/itwCredentialIssuanceUtils.ts 10.71% <ø> (ø)
...features/itwallet/common/utils/itwIssuanceUtils.ts 21.31% <ø> (ø)
...entials/saga/handleWalletCredentialsRehydration.ts 100.00% <100.00%> (ø)
...ures/itwallet/credentials/store/selectors/index.ts 60.00% <100.00%> (ø)
...Wallet/components/WalletCardsCategoryContainer.tsx 100.00% <100.00%> (ø)
...ures/newWallet/components/WalletCardsContainer.tsx 97.36% <100.00%> (+8.47%) ⬆️
...twallet/common/components/ItwWalletReadyBanner.tsx 88.88% <88.88%> (ø)
... and 3 more

... and 1347 files with indirect coverage changes


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 6010dea...ece3903. Read the comment docs.

Copy link

dpulls bot commented Sep 27, 2024

⚠️ Dpulls not installed on repository pagopa/io-react-native-wallet. Checkout our quickstart for how to install.

Copy link
Contributor

@LazyAfternoons LazyAfternoons left a comment

Choose a reason for hiding this comment

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

If we are not sure wether or not the SD-JWT includes the required fields, why don't we make them nullable and show a placeholder label which says that something went wrong?
I think that using the current date might be misleading and potentially it could shadow an error, even if that's a very remote possibility.

@gispada
Copy link
Collaborator Author

gispada commented Oct 2, 2024

If we are not sure wether or not the SD-JWT includes the required fields, why don't we make them nullable and show a placeholder label which says that something went wrong? I think that using the current date might be misleading and potentially it could shadow an error, even if that's a very remote possibility.

I wanted to avoid handling null values that are very unlikely, because both exp and iat should be included in SD-JWTs, as per specification. What about using the Unix epoch time as default instead of the current time?

@LazyAfternoons
Copy link
Contributor

LazyAfternoons commented Oct 2, 2024

If we are not sure wether or not the SD-JWT includes the required fields, why don't we make them nullable and show a placeholder label which says that something went wrong? I think that using the current date might be misleading and potentially it could shadow an error, even if that's a very remote possibility.

I wanted to avoid handling null values that are very unlikely, because both exp and iat should be included in SD-JWTs, as per specification. What about using the Unix epoch time as default instead of the current time?

I agree that they must be part of the SD-JWT. Maybe we can make the verifyAndParseCredential implementation thrown if they are not found during that step. I think we can fix this up later so they app doesn't have to handle null values or use defaults which might potentially shadow an issue.

Copy link

dpulls bot commented Oct 2, 2024

⚠️ Dpulls not installed on repository pagopa/io-react-native-wallet. Checkout our quickstart for how to install.

@gispada
Copy link
Collaborator Author

gispada commented Oct 2, 2024

I modified the way issuedAt is handled in d5223e9. There is a dependency on pagopa/io-react-native-wallet#145.

Copy link

dpulls bot commented Oct 2, 2024

⚠️ Dpulls not installed on repository pagopa/io-react-native-wallet. Checkout our quickstart for how to install.

Copy link
Contributor

@LazyAfternoons LazyAfternoons left a comment

Choose a reason for hiding this comment

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

There's an issue with the banner text:
IMG_A59940F7C3E9-1

@gispada
Copy link
Collaborator Author

gispada commented Oct 2, 2024

There's an issue with the banner text:

Unfortunately that is an issue with the Alert component from the design system. It is not perfectly centered with just one line.

@LazyAfternoons LazyAfternoons self-requested a review October 2, 2024 16:33
Copy link
Contributor

@LazyAfternoons LazyAfternoons left a comment

Choose a reason for hiding this comment

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

LGTM, let's wait for @mastro993 approve as well. Thanks.

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!

@mastro993 mastro993 merged commit f9a8b44 into master Oct 3, 2024
13 checks passed
@mastro993 mastro993 deleted the SIW-1665-eid-card-removal branch October 3, 2024 10:58
LazyAfternoons pushed a commit that referenced this pull request Oct 3, 2024
…creen (#6221)

> [!WARNING]  
> Depends on #6214

## Short description
This PR adds filters in the wallet screen. They are only visible when
the wallet contains at least one credential (not considering the eID).

## List of changes proposed in this pull request
- Filter tabs in the wallet screen with tests
- Reducer logic

## How to test
1. Disable the wallet instance: filters should not be visibile
2. Get the eID: filters should not be visible
3. Get another credential: filters should be visible

<img
src="https://github.com/user-attachments/assets/b0f2697e-6bf8-4819-b47b-f1d1fd9e6328"
width="400" />

---------

Co-authored-by: Federico Mastrini <[email protected]>
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.

4 participants