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-2016] Remove remote feature flag from linking options #6706

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

Conversation

mastro993
Copy link
Contributor

@mastro993 mastro993 commented Feb 11, 2025

Short description

This PR removes the remote feature flag from the IT Wallet linking options to allow routes to be configured properly at the app startup.

List of changes proposed in this pull request

  • Remove isItwEnabledSelector from useItwLinkingOptions
  • Added disabled prop to OperationResultScreenContent actions props
  • Added isItwEnabledSelector check on continuation CTA on every screen that is exposed by a deep link

How to test

Check that linking options for IT Wallet feature is working properly.
Disable the ITW feature flag and check that screens that are exposed by a deep link have all continuation CTAs disabled.

Copy link
Contributor

PR Title Validation for conventional commit type

All good! PR title follows the conventional commit type.

Copy link
Contributor

github-actions bot commented Feb 11, 2025

Jira Pull Request Link

This Pull Request refers to Jira issues:

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 49.99%. Comparing base (6e60dc2) to head (e6491d8).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...features/itwallet/navigation/ItwStackNavigator.tsx 16.66% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6706      +/-   ##
==========================================
+ Coverage   43.91%   49.99%   +6.08%     
==========================================
  Files        1417     1557     +140     
  Lines       30090    32449    +2359     
  Branches     6749     7345     +596     
==========================================
+ Hits        13213    16222    +3009     
+ Misses      16847    16176     -671     
- Partials       30       51      +21     
Files with missing lines Coverage Δ
...tures/itwallet/navigation/useItwLinkingOptions.tsx 100.00% <100.00%> (ø)
...features/itwallet/navigation/ItwStackNavigator.tsx 29.41% <16.66%> (-6.96%) ⬇️

... and 355 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 6e60dc2...e6491d8. Read the comment docs.

@mastro993 mastro993 self-assigned this Feb 12, 2025
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.

What happens if we start one of these flows and the feature is remotely disabled though? From a quick check in ItwIssuanceCredentialAsyncContinuationScreen.tsx it doesn't seem like it checks for that flag.

@mastro993
Copy link
Contributor Author

What happens if we start one of these flows and the feature is remotely disabled though? From a quick check in ItwIssuanceCredentialAsyncContinuationScreen.tsx it doesn't seem like it checks for that flag.

This is a good point. Keeping routes behind FF could lead to navigation errors, but if in the case FF is disabled we have no control over any deeplink interactions.
How could we solve this problem? Checking for the isItwActive flag on every screen that is exposed by a deeplink can be enough. This way we can prevent the user from continuing the flow at least. What do you think?

@LazyAfternoons
Copy link
Contributor

What happens if we start one of these flows and the feature is remotely disabled though? From a quick check in ItwIssuanceCredentialAsyncContinuationScreen.tsx it doesn't seem like it checks for that flag.

This is a good point. Keeping routes behind FF could lead to navigation errors, but if in the case FF is disabled we have no control over any deeplink interactions. How could we solve this problem? Checking for the isItwActive flag on every screen that is exposed by a deeplink can be enough. This way we can prevent the user from continuing the flow at least. What do you think?

I agree this might be the easiest solution to implement. Each screen which is a target of a deep link must check that. We could also implement a HOC which takes care of check it and showing an appropriate screen.

@mastro993
Copy link
Contributor Author

mastro993 commented Feb 13, 2025

What happens if we start one of these flows and the feature is remotely disabled though? From a quick check in ItwIssuanceCredentialAsyncContinuationScreen.tsx it doesn't seem like it checks for that flag.

This is a good point. Keeping routes behind FF could lead to navigation errors, but if in the case FF is disabled we have no control over any deeplink interactions. How could we solve this problem? Checking for the isItwActive flag on every screen that is exposed by a deeplink can be enough. This way we can prevent the user from continuing the flow at least. What do you think?

I agree this might be the easiest solution to implement. Each screen which is a target of a deep link must check that. We could also implement a HOC which takes care of check it and showing an appropriate screen.

This would be nice to have, but I can't figure out what should be the correct UX of the components/screens inside this HOC.
I found an easier solution by blocking (disabling) any CTA that allows the user to continue in the flow, here: 125ba40

I've found the way to use an HOC. If the FF is disabled we render a generic error screen. See here 15c2c6b

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