Enable auto suggestion for transfer input based off of users own transfer history #1964

netuoso · 2017-11-11T03:42:27Z

Issue

Due to the inability to turn on basic autocomplete in the transfer form, users were encountering issues and scams by sending transfers to users with the wrong name. This is largely due to the fact that users had to constantly retype the name of the users they were sending transfers to.

Solution

As a solution to this issue, I have enabled the ability for condenser to check the users transfer history and suggest accounts based off this information.

Additional Config Options

This implementation of the autosuggest transfer feature activates only when the first letter of the name is typed and matched with available results. On top of this, the results are limited to 10 matches at a time in an effort to reduce screen disruption.

Screenshots

bnchdrff

This looks great overall. I'd rather avoid adding the uniq dependency and instead building the list of usernames like this:

currentAccount.get('transfer_history').reduce((acc, cur) => ({...acc, cur: true}), {})

and then just Object.keys() it.

this way we don't add more library weight.

netuoso · 2017-11-13T21:58:59Z

I agree. I will make the suggested change.

netuoso · 2017-11-14T02:18:25Z

Ok I was able to remove the need for uniq by using .filter(function(e,i,arr) {return arr.lastIndexOf(e) === i});

Wasn't able to get reduce to work the way I wanted. Maybe you can modify the filter to use reduce if you prefer. However this does result in the expected result of a sanitized array.

TimCliff · 2017-11-14T15:00:29Z

One use case I can think of where this would cause a problem is if a user had already transferred to a bad/wrong account, it would keep showing up in their list going forward. Could it at least check the list against the blacklisted accounts? Hopefully that will cover the worst scenarios.

bnchdrff · 2017-11-14T16:22:27Z

what if we weighted a user's followed accounts to the top of the autocomplete list & denoted them as such?

TimCliff · 2017-11-14T16:38:33Z

That would be good to do too.

netuoso · 2017-11-16T06:18:09Z

Great suggestions. Thanks for actually taking this one into consideration. I have been getting an immense benefit from the alpha version.

bnchdrff · 2017-11-16T22:19:21Z

alright i've got this nice & cleaned up; switched to a more accessible autocomplete component, autocomplete includes followed users as well as previous transfers along with the # of times you transferred to them

todo:

on change, query steemd & see if the user has a negative rep and if so, display a warning
make sure it works with many many autocomplete & truncate if necessary
get ux / style feedback from @pkattera
generalize the autocomplete into a wrapper component maybe

bnchdrff · 2017-11-17T15:01:15Z

to be clear, i think the only two immediate & critical issues are truncating the list & style feedback; the other two things can happen in later PRs

bnchdrff · 2017-11-27T14:53:17Z

src/app/components/modules/Transfer.jsx

+        const transferToLog = currentAccount.get('transfer_history').reduce((acc, cur) => {
+            if (cur.getIn([1, 'op', 0]) === 'transfer') {
+                const username = cur.getIn([1, 'op', 1, 'to']);
+                const numTransfers = acc.get(username) ? acc.get(username).numTransfers + 1 : 1


bnchdrff · 2017-12-08T19:13:54Z

i've rebased this PR; i'm happy with where it's at. @relativityboy take a peek.

relativityboy

Please update this PR's functionality in line with the above.

Also, with the latest rebase to master we're getting some errors in Autocomplete. That should be dealt with as well.

relativityboy · 2017-12-14T01:15:45Z

src/app/components/modules/Transfer.jsx

+            .toMap()
+            .map(username => ({ username, label: labelFollowingUser }))
+            .merge(transferToLog)
+            .sortBy(u => u.numTransfers)


@bnchdrff @netuoso - this sort doesn't work.

are you saying we shouldn't sort by the number of previous transfers, or that it actually doesn't work? it's working for me:

ah found a condition where it doesn't work, fixing it now...

It didn't sort at all on live data. .sortBy return(s/ed) a list that has the same order as this.props.following.

There were 2 transfers in the list generated by .merge(transferToLog) with the rest being undefined. Neither of those had their order changed. The comparator fn will prob need to move to an a > b? 1 : (a < b? : -1 : 0) operation.

relativityboy · 2017-12-14T01:16:28Z

src/app/components/modules/Transfer.jsx

+            .merge(transferToLog)
+            .sortBy(u => u.numTransfers)
+            .reverse()
+            .toArray();


We need to add a limit to 10 users (as suggested by @bnchdrff )

bnchdrff · 2017-12-17T18:46:07Z

ok, made some changes: limited to 20 followed users & 20 transfer history users, and refactored to a method that only runs once (no need for a wrapper quite yet, we can do that if this has to be reused somewhere else)

bnchdrff · 2017-12-17T20:32:17Z

i'm not sure how to do the negative rep thing quickly/easily ... i'd rather put that part into another PR since it's another boatload of work.

the getComputedStyle error is tricky; it's in react-autocomplete and i'm investigating it now.

bnchdrff · 2017-12-18T14:41:16Z

the getComputedStyle is actually caused by our webpack config; we are overwriting global in dev.config.js https://github.com/steemit/condenser/blob/master/webpack/dev.config.js#L29 & prod.config.js https://github.com/steemit/condenser/blob/master/webpack/prod.config.js#L14

these were both defined in Initial commit 😠 06c90aa#diff-bb7d85ed7641f411d08dfc8a989a1dabR40 06c90aa#diff-5f694022cd10cae14c85a89025b45c45R64

things seem to run fine if i remove them, and there are no refs to this variable in our sourcetree; @valzav @roadscape @relativityboy any idea what the purpose of global.TYPED_ARRAY_SUPPORT might be?

relativityboy · 2017-12-18T15:59:09Z

No idea

bnchdrff · 2017-12-18T16:10:11Z

just checked if TYPED_ARRAY_SUPPORT is used anywhere else in the code back at Initial commit -- it's not!

bnchdrff · 2017-12-18T16:18:27Z

bc@zebrina:~/steemit/condenser$ git log -GTYPED_ARRAY_SUPPORT 
commit 01669549774bba961a72d4d64ae8a2332c034f6c
Author: Valentine Zavgorodnev <[email protected]>
Date:   Thu Oct 12 16:46:41 2017 -0400

    Upgrade to webpack3 (#1797)
    
    * only webpack related changes from webpack3 branch
    
    * update steem-js in order to use http json rpc
    
    * move imports to the top (eliminate codeclimate warning)
    
    * bump npm version to 5.4.2

commit 06c90aa8260f09c4ae061e652d884f68b8a6a409
Author: James Calfee <[email protected]>
Date:   Thu Jul 28 09:58:41 2016 -0400

    Initial commit

roadscape · 2017-12-18T16:18:54Z

Grepping node_modules, it seems this flag is used by buffer. @valzav added these flags originally, let's check with him.

relativityboy · 2017-12-18T19:01:22Z

This sorting on numTransfers now. That's really good. But the slice is happening too soon. This could result in significantly sub-optimal results for users with lots of transfers & followings.

User with just a few followings.

Same user with 20+ followings.

relativityboy · 2017-12-18T19:10:09Z

I've updated the sortBy call to order by numTransfers desc, then username asc, and removed the trucate/slice because the single render means a user won't be prompted for their end-of-alphabet followings. @bnchdrff requesting 👍 / 👎 feedback

Same user, same dataset.

bnchdrff · 2017-12-18T19:14:17Z

@relativityboy looks fine to me; you aren't using that const yet though!

bnchdrff · 2017-12-18T19:16:10Z

if you want to use a const for that one truncation, also pls use it for the other one as well

roadscape · 2017-12-18T20:26:58Z

Implementation looks good to me, this will be a useful feature. Reliance on transfers list is fine for now, we can leverage better APIs in the future to populate the autocomplete.

Before merging we need clarification from @valzav why TYPED_ARRAY_SUPPORT was added; it's referenced by buffer which touches potentially critical functions such as signing.

valzav · 2017-12-18T21:50:41Z

@roadscape, I think you are right - TYPED_ARRAY_SUPPORT was needed for the buffer lib, it must be James who added it (I most probably just helped him with webpack's config). Not sure if it's steel needed. I would suggest to try to remove and check if it breaks anything.

roadscape · 2017-12-18T22:24:48Z

Thanks Val, James' commits around that one have to do with adding a QR reader which has since been disabled. I also found a reference to this flag being related to browser compatibility issues with typed arrays (iOS/Safari in that case).

Browser support for typed arrays looks better currently: https://caniuse.com/#feat=typedarrays

In condenser, buffer seems to be mainly involved in these operations:

login
posting
image upload

bnchdrff · 2017-12-18T22:47:17Z

i'm testing those ops now, and also looking through the code for other Buffer usage

bnchdrff · 2017-12-18T23:51:58Z

tested login, image upload, posting, and account creation -- no errors found!

remove unused global override see discussion at #1964 (comment)

bnchdrff reviewed Nov 12, 2017

View reviewed changes

bnchdrff self-assigned this Nov 14, 2017

This was referenced Nov 16, 2017

warn before transfer to negative rep account #1781

Closed

Autocomplete=On For Wallet Transfer "To:" Field #1893

Closed

bnchdrff requested a review from relativityboy November 17, 2017 22:51

bnchdrff mentioned this pull request Nov 19, 2017

Enable a check on the transfer form to ensure users are not sending transfers to themselves #2028

Closed

bnchdrff reviewed Nov 27, 2017

View reviewed changes

bnchdrff assigned relativityboy and unassigned bnchdrff Dec 8, 2017

relativityboy suggested changes Dec 14, 2017

View reviewed changes

relativityboy approved these changes Dec 18, 2017

View reviewed changes

Add autocomplete for transfer "to" field

0bc0139

remove unused global override see discussion at #1964 (comment)

bnchdrff merged commit c859833 into steemit:master Dec 19, 2017

bnchdrff mentioned this pull request Dec 20, 2017

Warning to Input Memo When Sending to Exchanges is Not Displaying #2189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable auto suggestion for transfer input based off of users own transfer history #1964

Enable auto suggestion for transfer input based off of users own transfer history #1964

netuoso commented Nov 11, 2017

bnchdrff left a comment

netuoso commented Nov 13, 2017

netuoso commented Nov 14, 2017

TimCliff commented Nov 14, 2017 •

edited

Loading

bnchdrff commented Nov 14, 2017

TimCliff commented Nov 14, 2017

netuoso commented Nov 16, 2017

bnchdrff commented Nov 16, 2017 •

edited

Loading

bnchdrff commented Nov 17, 2017

bnchdrff Nov 27, 2017

bnchdrff commented Dec 8, 2017

relativityboy left a comment

relativityboy Dec 14, 2017

bnchdrff Dec 17, 2017

bnchdrff Dec 17, 2017

relativityboy Dec 18, 2017

relativityboy Dec 14, 2017

bnchdrff commented Dec 17, 2017 •

edited

Loading

bnchdrff commented Dec 17, 2017

bnchdrff commented Dec 18, 2017

relativityboy commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

roadscape commented Dec 18, 2017

relativityboy commented Dec 18, 2017 •

edited

Loading

relativityboy commented Dec 18, 2017 •

edited

Loading

bnchdrff commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

roadscape commented Dec 18, 2017

valzav commented Dec 18, 2017

roadscape commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

Enable auto suggestion for transfer input based off of users own transfer history #1964

Enable auto suggestion for transfer input based off of users own transfer history #1964

Conversation

netuoso commented Nov 11, 2017

Issue

Solution

Additional Config Options

Screenshots

bnchdrff left a comment

Choose a reason for hiding this comment

netuoso commented Nov 13, 2017

netuoso commented Nov 14, 2017

TimCliff commented Nov 14, 2017 • edited Loading

bnchdrff commented Nov 14, 2017

TimCliff commented Nov 14, 2017

netuoso commented Nov 16, 2017

bnchdrff commented Nov 16, 2017 • edited Loading

bnchdrff commented Nov 17, 2017

bnchdrff Nov 27, 2017

Choose a reason for hiding this comment

bnchdrff commented Dec 8, 2017

relativityboy left a comment

Choose a reason for hiding this comment

relativityboy Dec 14, 2017

Choose a reason for hiding this comment

bnchdrff Dec 17, 2017

Choose a reason for hiding this comment

bnchdrff Dec 17, 2017

Choose a reason for hiding this comment

relativityboy Dec 18, 2017

Choose a reason for hiding this comment

relativityboy Dec 14, 2017

Choose a reason for hiding this comment

bnchdrff commented Dec 17, 2017 • edited Loading

bnchdrff commented Dec 17, 2017

bnchdrff commented Dec 18, 2017

relativityboy commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

roadscape commented Dec 18, 2017

relativityboy commented Dec 18, 2017 • edited Loading

relativityboy commented Dec 18, 2017 • edited Loading

bnchdrff commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

roadscape commented Dec 18, 2017

valzav commented Dec 18, 2017

roadscape commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

bnchdrff commented Dec 18, 2017

TimCliff commented Nov 14, 2017 •

edited

Loading

bnchdrff commented Nov 16, 2017 •

edited

Loading

bnchdrff commented Dec 17, 2017 •

edited

Loading

relativityboy commented Dec 18, 2017 •

edited

Loading

relativityboy commented Dec 18, 2017 •

edited

Loading