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

DashPay: Send Payment - Pending Contact (NMA-373) #503

Merged
merged 41 commits into from
Sep 29, 2020

Conversation

HashEngineering
Copy link
Collaborator

@HashEngineering HashEngineering commented Sep 3, 2020

Issue being fixed or feature implemented

NMA-373
This must be merged (and preferably rebased) after #501 .

This PR is not complete:

  • Contacts Screen, show Add New Contact if there are no contacts or pending contact requests. Previously, if a contact request was sent, then the Contacts Screen was blank even though there were no pending/contacts to display.

  • Layout issues addressed on DashPay User Screen for a user that has sent a contact request

Related PR's and Dependencies

none

Screenshots / Videos

How Has This Been Tested?

  • QA (Mobile Team)

Checklist:

  • I have performed a self-review of my own code and added comments where necessary
  • I have added or updated relevant unit/integration/functional/e2e tests

desamtralized and others added 30 commits July 28, 2020 22:29
previous code didn't show Add New Contact if we had sent a request, though we had no contacts.
…#509)

* Simplified tracking of the progress of send contact request operation
Removed some debug code

* No need to derive encryption key one more time

* Improved exception handling
Added tracking the progress after sending contact request.
…ndling status of contact requests.

Fixed issues with sending payments to strangers.
@tomasz-ludek tomasz-ludek self-assigned this Sep 24, 2020
# Conflicts:
#	wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt
@tomasz-ludek tomasz-ludek marked this pull request as ready for review September 24, 2020 17:55
Comment on lines 20 to 25
@Query("SELECT * FROM dashpay_profile where username = :username")
fun loadFromUsername(username: String): LiveData<DashPayProfile?>

fun loadFromUsernameDistinct(userId: String):
LiveData<DashPayProfile?> = loadFromUsername(userId).getDistinct()

Copy link
Collaborator Author

@HashEngineering HashEngineering Sep 24, 2020

Choose a reason for hiding this comment

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

This looks like my code. It doesn't match that of DashPayProfileDaoAsync which has better names.

These methods should be called loadByUsername and loadByUsernameDistinct

We are missing a loadByUserId here compared to DashPayProfileDaoAsync, it is still called load.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing loadByUserId added

import androidx.lifecycle.LiveData
import de.schildbach.wallet.WalletApplication

abstract class ContactsBasedLiveData<T>(val walletApplication: WalletApplication,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea to refactor this code out to a new class, rather than have duplicate code in many classes.

Copy link
Collaborator Author

@HashEngineering HashEngineering left a comment

Choose a reason for hiding this comment

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

This was tested on a few devices, by clicking and scanning QR codes. Overall it works well:

Here are some bugs:

  1. When sending a contact request to a user, once the operation is completed "Activity" appears but there is no activity listed. The only item would be that the contact request was sent on a specific date/time. The activity_rv has the "Activity" row item, but not a Notification item. This problem also existed before you work on this story, though the "Activity" Row was also missing.
  2. The Accept button on a contact or notification row on other screens (Contacts, Notifications, Add a Contact) does nothing when pressed.
  3. One comment was added to the code to align method names between the DashPayProfileDao* classes
  4. Merge with evonet-develop and fix one compile issue related to loadDistinct and loadByUserNameDistinct - this is from merging NMA-431 | Settings | View DashPay Profile #512.
  5. Sending to a regular address (not a contact or a user), may not allow the close button to work on the TransactionResultActivity.

Mostly changed the naming convention for Room DAOs (the `Async` suffix should not be used in coroutine based / suspended versions)
# Conflicts:
#	wallet/src/de/schildbach/wallet/ui/dashpay/ContactSearchResultsAdapter.kt
#	wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt
Comment on lines 42 to 49
// val userLiveData by lazy {
// AppDatabase.getAppDatabase().dashPayProfileDaoAsync().loadByUserIdDistinct(userData.dashPayProfile.userId).switchMap {
// return@switchMap liveData(Dispatchers.IO) {
// userData = platformRepo.loadContactRequestsAndReturn(it)!!
// emit(userData)
// }
// }
// }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed

Comment on lines 63 to 65
// val userLiveDataObservable by lazy {
// AppDatabase.getAppDatabase().dashPayProfileDaoAsync().loadDistinct(userData.dashPayProfile.userId)
// }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed

Comment on lines 81 to 98
// fun sendContactRequest(refreshUserData: Boolean) {
// viewModelScope.launch(Dispatchers.IO) {
// try {
// val toUserId = userLiveData.value!!.dashPayProfile.userId
// val username = userLiveData.value!!.username
// val result = platformRepo.sendContactRequest(toUserId)
// if (refreshUserData) {
// userLiveData.postValue(platformRepo.getUser(userData.username).first())
// } else {
// userLiveData.value!!.toContactRequest = result
// userLiveData.postValue(userLiveData.value) //notify observers
// }
// } catch (ex: Exception) {
// log.error(ex.message, ex)
// userLiveData.postValue(userLiveData.value) //notify observers
// }
// }
// }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed

// AppDatabase.getAppDatabase().dashPayProfileDaoAsync().loadDistinct(userData.dashPayProfile.userId)
// }

fun a() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is there a better name for this method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was a test code.
Removed

Comment on lines 108 to 115
// background color alternates based on first letter
val colorResId = if (item.usernameSearchResult.dashPayProfile.username[0].toLowerCase().toInt() % 2 != 0) {
R.color.white
} else {
R.color.dash_lighter_gray
}
val color = ResourcesCompat.getColor(holder.itemView.resources, colorResId, null)
setBackgroundColor(color)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This alternating background color code was removed in a merged PR from last week:

https://github.com/dashevo/dash-wallet/pull/514/files

Copy link
Collaborator

@tomasz-ludek tomasz-ludek Sep 29, 2020

Choose a reason for hiding this comment

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

This explains my confusion when merging evonet-develop into my code.
Fixed

@HashEngineering
Copy link
Collaborator Author

Review:

Other issues in my previous "Review" were addressed. Only things that remain are commented code and the a()

@HashEngineering HashEngineering merged commit 0e687ea into evonet-develop Sep 29, 2020
@tomasz-ludek tomasz-ludek deleted the dashpay-pay-pending branch October 30, 2020 14:23
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.

3 participants