-
Notifications
You must be signed in to change notification settings - Fork 14
feat(ui): Add joint account support to existing account screens #516
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
base: multisig/05-data-models
Are you sure you want to change the base?
feat(ui): Add joint account support to existing account screens #516
Conversation
014714a to
c3e316e
Compare
4b6f926 to
f44cf3b
Compare
c3e316e to
6b1b4d5
Compare
f44cf3b to
c787611
Compare
- Update AccountIconResource with JOINT type - Add joint account badge to account list items - Update account detail screens for joint accounts - Add joint account icon drawable preview - Update account type detection in existing flows - Add TransactionSigner.Joint handling
6b1b4d5 to
a997278
Compare
c787611 to
8bd9179
Compare
…-support Resolve merge conflicts: - DefaultAccountDetailAccountsItemProcessor: keep 06's joint account features with HasInboxItemsForAddress - AccountStatusDetailPreviewDecider: keep 06's proper Joint account handling - Restore use cases removed by merge: GetJointAccount, GetJointAccountUseCase, GetJointAccountParticipantCount, GetJointAccountParticipantCountUseCase - Update DefaultJointAccountDetailProcessor to use InboxMessages instead of InboxMessagesDTO
- Add internal modifier to AssetInboxRepositoryImplTest - Update HasInboxItemsForAddressUseCaseTest to use AssetInbox and JointAccount instead of DTO versions
…unt bindings Add Dagger @provides methods for GetJointAccount and GetJointAccountParticipantCount use cases in JointAccountModule to fix missing binding errors.
| override suspend fun invoke(address: String): Int { | ||
| val jointAccount = getLocalAccount(address) as? LocalAccount.Joint | ||
| return jointAccount?.participantAddresses?.size ?: 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done in repository. JointAccountRepository can return this value. No need to use getLocalAccount
| ) : GetJointAccount { | ||
|
|
||
| override suspend fun invoke(address: String): LocalAccount.Joint? { | ||
| return getLocalAccount(address) as? LocalAccount.Joint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JointAccountRepository can return this instead of calling getLocalAccount
| val participants: List<JointAccountParticipantItem>, | ||
| val participantAddresses: List<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these contain the same addresses? If so, we shouldn't use different parameters for them
| stateDelegate.updateState { contentState } | ||
| } | ||
|
|
||
| private suspend fun getInvitationData(): JointAccountDetailProcessor.InvitationData? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function name is not only doing what it is supposed to do, it also updates state.
| private val contactRepository: ContactRepository | ||
| ) : CreateJointAccountParticipantItem { | ||
|
|
||
| override suspend fun getLocalAccountAddresses(): List<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no point of this function. We can inject getLocalAccountAddresses into where we call this function.
| class JointAccountInboxOperationsUseCase @Inject constructor( | ||
| private val deviceIdUseCase: DeviceIdUseCase, | ||
| @param:Named(JointAccountRepository.INJECTION_NAME) | ||
| private val jointAccountRepository: JointAccountRepository, | ||
| ) { | ||
|
|
||
| fun getDeviceId(): String? = deviceIdUseCase.getSelectedNodeDeviceId() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary use case
|
|
||
| package com.algorand.android.modules.accountdetail.jointaccountdetail.ui.model | ||
|
|
||
| data class JointAccountDetailUiState( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused class
| override fun onEditAddressClick(address: String) { | ||
| viewModel.onEditContactClick(address) | ||
| } | ||
|
|
||
| override fun onCopyAddressClick(address: String) { | ||
| onAccountAddressCopied(address) | ||
| } | ||
|
|
||
| override fun onIgnoreClick() { | ||
| viewModel.onIgnoreClick() | ||
| } | ||
|
|
||
| override fun onAddClick() { | ||
| viewModel.onAddClick() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do these viewmodel calls in screen composable, since we already pass viewmodel. There is no value of sending these events to Fragment and call VM function.
| import javax.inject.Inject | ||
|
|
||
| // ISO-8601 ISO_DATE_TIME | ||
| class InboxLastOpenedTimeLocalSource @Inject constructor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use PersistentCacheProvider instead of shared pref local source
| val eligibleSigners = participantAddresses.mapNotNull { address -> | ||
| allLocalAccounts.find { it.algoAddress == address } | ||
| }.filter { localAccount -> | ||
| localAccount is LocalAccount.Algo25 || localAccount is LocalAccount.HdKey | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary iterations. Can be use single iteration. Also we can create a db query instead of getting all the accounts every single time
…-support * multisig/05-data-models: Address PR #515 comments for data models Fix detekt issues without @Suppress Fix pre-existing detekt issues Use Go SDK for transaction signing and return signature directly fix: Address remaining PR #514 review comments fix: Address PR #514 review comments fix: Address PR #514 feedback docs: Add refactoring restrictions to prevent unwanted changes
Summary
AccountIconResource.JOINTwith dedicated icon stylingJointAccountBadgeViewHolderfor account list itemsJointAccountDetailFragmentandJointAccountDetailScreenJointAccountDetailViewModelwith processor patternGetAccountIconDrawablePreviewfor joint accountsTest Plan