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

feat: normalize redux state with short tokens #2365

Merged
merged 11 commits into from
May 26, 2021

Conversation

thi4go
Copy link
Member

@thi4go thi4go commented May 1, 2021

This diff normalizes our entire redux state to use short tokens instead of full tokens, resolving various bugs that may happen when the app expects one token form, and receives another.

closes #2353

@amass01
Copy link
Member

amass01 commented May 1, 2021

@thi4go thanks for this mate, this should be closing #2351 I think.

@tiagoalvesdulce
Copy link
Member

We should wait for e2e to be ready before getting this in since it touches a lot of sensitive parts of the app

@thi4go thi4go changed the title [wip] Normalize redux state to use short tokens throughout the app Normalize redux state with short tokens May 7, 2021
@thi4go thi4go changed the title Normalize redux state with short tokens feat: normalize redux state with short tokens May 7, 2021
@thi4go thi4go marked this pull request as ready for review May 7, 2021 21:18
@thi4go
Copy link
Member Author

thi4go commented May 8, 2021

Hey guys, this is ready for a review. Please check the state tree to see if we are indeed consistent about short token use on all of the trees :)

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

Code looks good. Couldn't find any error with a quick test. I'll wait #2383 to merge this so we can be more confident it doesn't introduce issues we couldn't catch.

@tiagoalvesdulce
Copy link
Member

Closed by mistake

@thi4go thi4go force-pushed the normalize-redux branch 2 times, most recently from 8f27636 to 6c3056c Compare May 12, 2021 20:52
@thi4go
Copy link
Member Author

thi4go commented May 12, 2021

@tiagoalvesdulce e2e already being very useful. caught some minor bug that could otherwise pass unoticed on a review 👍 :)

this PR is ready

@tiagoalvesdulce
Copy link
Member

tiagoalvesdulce commented May 13, 2021

Nice! Code is looking good, just run yarn prettify and this should be good to go

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

I ran prettify for thiago because his dev env was messed up.

While testing this I found several Issues. See:

Duplicated new proposal and proposal not showing on top of discussion tab:

bug.mp4

Wrong tabs:
Screen Shot 2021-05-17 at 10 24 31
Screen Shot 2021-05-17 at 10 25 01
Screen Shot 2021-05-17 at 10 25 18
Screen Shot 2021-05-17 at 10 25 25

Voting proposal showing up at admin's unreviewed tab:

Screen Shot 2021-05-17 at 10 26 06

@thi4go
Copy link
Member Author

thi4go commented May 17, 2021

Thanks for the review @tiagoalvesdulce, nice catch. Will be fixing this asap

@tiagoalvesdulce
Copy link
Member

I think the sorting is still messed up in the "In Discussion" tab. I don't know if it's intentional but it's putting proposals with voting authorized on top of others:

Screen Shot 2021-05-25 at 18 30 38

The proposal "sssssss" was created after the proposal "oooooooooo".

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

Sorting is working as expected now.

Seeing this error when trying to submit an edited proposal:

Screen Shot 2021-05-26 at 10 21 48

Is the endpoint expecting the full token?

src/components/RecordsView/RecordsView.jsx Outdated Show resolved Hide resolved
Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

LGTM

@tiagoalvesdulce tiagoalvesdulce merged commit 676039c into decred:master May 26, 2021
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.

Use short tokens in redux normalized state.
3 participants