Increase flexibility of ForageElement hierarchy #248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @devinmorgan and the rest of your teammates on |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
forage-android/src/main/java/com/joinforage/forage/android/core/ui/element/ForagePanElement.kt
Dismissed
Show dismissed
Hide dismissed
forage-android/src/main/java/com/joinforage/forage/android/core/ui/element/ForagePanElement.kt
Dismissed
Show dismissed
Hide dismissed
forage-android/src/main/java/com/joinforage/forage/android/core/ui/element/ForagePanElement.kt
Dismissed
Show dismissed
Hide dismissed
forage-android/src/main/java/com/joinforage/forage/android/core/ui/element/ForagePinElement.kt
Dismissed
Show dismissed
Hide dismissed
forage-android/src/main/java/com/joinforage/forage/android/core/ui/element/ForagePinElement.kt
Dismissed
Show dismissed
Hide dismissed
forage-android/src/main/java/com/joinforage/forage/android/core/ui/element/ForagePinElement.kt
Dismissed
Show resolved
Hide resolved
devinmorgan
force-pushed
the
devin/move-things-to-core-and-ecom
branch
from
6458699 to
1772e21
Compare
devinmorgan
force-pushed
the
devin/fx-1331-introduce-the-new-forageelement-hierarchy-from-pos-repo-into
branch
from
08b79c4 to
bc613e2
Compare
devinmorgan
requested review from
owenkim,
ForageRob,
dleis612,
djoksimo and
evanfreeze
as code owners
djoksimo
requested changes
May 10, 2024
There was a problem hiding this comment.
Great stuff @devinmorgan ! Left a few comments. Open to discussing them sync/async!
forage-android/src/main/java/com/joinforage/forage/android/core/ForageSDKInterface.kt
Show resolved
Hide resolved
| if (vaultResponse.isSuccess) return null | ||
| val resRegExp = BtResponseRegExp(vaultResponse) | ||
| return try { | ||
| // TODO: add DD metric to track frequency of proxy errors |
There was a problem hiding this comment.
The caller captures proxy errors using the vault monitor/measurer [VaultProxyResponseMonitor]. Can remove or update this comment to include the context from this comment
There was a problem hiding this comment.
Ah nice! OK I'll modify the comment to capture this. Thanks!
There was a problem hiding this comment.
Updated the comment!
...droid/src/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BTResponseParser.kt
Show resolved
Hide resolved
...rc/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BasisTheoryPinSubmitter.kt
Show resolved
Hide resolved
...droid/src/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BtResponseRegExp.kt
Outdated
Show resolved
Hide resolved
...oid/src/main/java/com/joinforage/forage/android/ecom/services/vault/vgs/VGSResponseParser.kt
Show resolved
Hide resolved
devinmorgan
commented
May 10, 2024
forage-android/src/main/java/com/joinforage/forage/android/core/ui/element/ForagePinElement.kt
Outdated
Show resolved
Hide resolved
dleis612
approved these changes
May 15, 2024
We want the private pos repo and the public ecom repo to have the ability to evolve separately accordingly their unique needs while still sharing common code. The first step in this journey is to make the public ecom repo completely unaware of the existence of the private pos repo. We do that by deleting all of the pos code Signed-off-by: Devin Morgan <[email protected]>
The second step in the public ecommerce repo that needs to take place in order to make sharing code between the private pos repo and this repo pain free; all while allowing for both repos to evolve independently Signed-off-by: Devin Morgan <[email protected]>
This is the third step in the public ecommerce repo that needs to take place in order to make sharing code between the private pos repo and this repo pain free; all while allowing for both repos to evolve independently. This hierarchy will make it easy to keep ecom things in the ecom-ForagePinEditText and pos things in the pos-ForagePINEditText Signed-off-by: Devin Morgan <[email protected]>
Previously `vaultToForageResponse` relied on generics to because
VGS and BT responses differ. However, generics in Kotlin do not
support the union ("|") types, which means that using generics
will require the parent `AbstractVaultSubmitter` class to be aware
of all classes that implement it.
This is a problem because `AbstractVaultSubmitter` houses the
common template for of submitting requests to vaults and ought
to live in `/core`. But `/core` cannot be aware of any vault
specific details if it is to be reusable by the pos repo.
So, this commit, replaces the usage of generics with a common
interface that all VaultSubmitter may _privately_ implement. The
ability for VaultSubmitters to _privately_ implement the details
by which they interpret vault response and
ForageApiResponse.Success or ForageApiResponse.Failures makes it
possible to move `AbstractVaultSubmitter` to `/core` and still
house the common logic for submitting requests to vaults
Signed-off-by: Devin Morgan <[email protected]>
Signed-off-by: Devin Morgan <[email protected]>
devinmorgan
force-pushed
the
devin/move-things-to-core-and-ecom
branch
from
1772e21 to
ee43184
Compare
devinmorgan
force-pushed
the
devin/fx-1331-introduce-the-new-forageelement-hierarchy-from-pos-repo-into
branch
from
baf176a to
09cfb94
Compare
Signed-off-by: Devin Morgan <[email protected]>
devinmorgan
changed the base branch from
devin/move-things-to-core-and-ecom
to
main
djoksimo
approved these changes
May 15, 2024
devinmorgan
deleted the
devin/fx-1331-introduce-the-new-forageelement-hierarchy-from-pos-repo-into
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Step 3 of Part 1.. This is the top of the stack.
Why
See here
Test Plan
Demo
N/A - no new functionality
How
This PR is the top-of-the-stack is that will get merged into
main.Once folks have approved the stack. I will change the base of this PR to point tomainand merge that in. I then will close #246 and #247. See here for more detail.