Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Fix autocomplete when changing a vote - Closes #990 #1000

Conversation

pouljohn1
Copy link

@pouljohn1 pouljohn1 commented Nov 22, 2017

What was the problem?

Described here #990

How did I fix it?

Previously when we had toggled and untoggled specified vote, reducer state was with false state (vote.confirmed and vote.unconfirmed were false). I've changed it in a way that when user untoggled vote and state of this vote is unconfirmed I'm removing this vote from state, so final state is the same as initial state (before toggling vote).

How to test it?

Described here #990

Review checklist

@slaweet slaweet self-assigned this Nov 22, 2017
@slaweet slaweet requested a review from reyraa November 22, 2017 09:15
@reyraa reyraa changed the base branch from development to 1.3.0 November 24, 2017 07:51
@reyraa
Copy link
Contributor

reyraa commented Nov 24, 2017

We appreciate your participation. Please consider the following tweak. in src/utils/api/delegate.js#L26, apply this change:

return new Promise((resolve, reject) =>
    listDelegates(activePeer, options)
      .then((response) => {
        resolve(response.delegates.filter(delegate =>
          Object.keys(votedDict).filter(item =>
            (item === delegate.username) && (item.confirmed || item.unconfirmed)).length === 0,
        ));
      })
      .catch(reject),
  );

I've added item.confirmed || item.unconfirmed to the condition and it fixes the issue.

@slaweet slaweet changed the title Fixed autocomplete changing a vote returns no matches for a valid delegate Fix autocomplete when changing a vote - Closes #990 Nov 24, 2017
@pouljohn1
Copy link
Author

It was my first attempt, but for me it is more like hack. As I understand state like !unconfirmed and !confirmed should not happen (what does it mean than?) - for me it should be either confirmed or unconfirmed.
My approach results the same state as it was before toggling vote on which I think is a good approach.

But of course I don’t know whole code so maybe missing something. If you want I could change to this.

@slaweet
Copy link
Contributor

slaweet commented Nov 24, 2017

I see that the usage of unconfirmed and confirmed is not very clear.

  • unconfirmed says if the delegate is selected or not in the app.
  • confirmed says if the delegate is selected or not on the blockchain.

!unconfirmed and !confirmed means that the delegate selected neither in the app nor on the blockchain.

As @reyraa mentioned the bug can be fixed with much fewer changes in this function:
https://github.com/LiskHQ/lisk-nano/blob/32cc92608f5c6c72dacf1f286d267ac2111a7943/src/utils/api/delegate.js#L26-L35

@pouljohn1
Copy link
Author

I get this, but still I think that such vote should not be in reducer (it means the same). For me this reducer needs some refactor, it is too much error prone.
But for not making too much changes I will do the way that you told

@slaweet slaweet closed this Nov 27, 2017
@slaweet slaweet changed the base branch from 1.3.0 to 2.0.0 November 30, 2017 15:57
@slaweet slaweet reopened this Nov 30, 2017
@slaweet
Copy link
Contributor

slaweet commented May 23, 2018

The proposed solution was implemented in #1074 so closing this one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants