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

[cms-v2]Inital work for cms refactoring #1625

Merged
merged 84 commits into from Jan 15, 2020

Conversation

fernandoabolafio
Copy link
Member

@fernandoabolafio fernandoabolafio commented Dec 16, 2019

This PR is the first of many to come to accomplish what is described in #1604.

This PR contains the following features:

  • Authentication
    • Login
    • Signup
    • Reset password
    • Resend verification email
  • Invoice
    • User List
    • Admin List
    • New
    • Admin actions (approve, reject, dispute)
    • Comments
  • User/Contractor
    • Invite
    • Manage (type, domain, supervisors)

This is a preview of how the main screens are currently looking with the refactoring:

Screenshot 2019-12-03 at 16 22 50

Screenshot 2019-12-03 at 16 25 58

Depends on decred/politeia#1049
Depends on decred/politeia#1089

obs: This PR was firstly open on #1605. It was mistakenly merged in the branch cmsv2 so I had to reopen it with a fresh PR. Sorry for the confusion.

@fernandoabolafio
Copy link
Member Author

fernandoabolafio commented Jan 9, 2020

@thi4go I addressed all of your review comments. Regarding:

I get a Error: The provided signature was invalid. from the api when I try to do any admin action. Need to check this again on my linux env, but would be nice if you tested it there as well.

This won't happen anymore once decred/politeia#1089 is in. You can verify that this error doesn't happen when you try to apply the actions in the invoice detail page instead of on the list page.

@thi4go
Copy link
Member

thi4go commented Jan 9, 2020

Hey @fernandoabolafio, looking very good on my end, tested the listed features and they are all working fine. Just those two minor comments:

  • The description needs to reset after each new row
  • I'm not sure how this is being done, but I wasn't able to set a supervisor to a user from my admin account.

Other than this, the PR is LGTM.

@fernandoabolafio
Copy link
Member Author

@thi4go can you describe what was happening when you tried to set a supervisor? Perhaps a small video or a gif would be nice.

I'm not sure how this is being done, but I wasn't able to set a supervisor to a user from my admin account.

@thi4go
Copy link
Member

thi4go commented Jan 10, 2020

@fernandoabolafio no users were being listed as possible supervisors for my users, maybe because I only had one admin account, or a incorrect user status to make it possible.

Copy link
Member

@victorgcramos victorgcramos left a comment

Choose a reason for hiding this comment

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

Hi Fernando, good job! This new version of CMS is looking really great! Congratulations 👏 .

Just a few comments after testing it on mobile (safari for iOS 13.3):

  1. Mobile view needs some left padding for dropdown menu options, see:

  2. Also, on mobile view, at least for Safari 13, I was not able to edit the subdomain field. All other fields are okay.

  3. The manage user tab should be only available for admins.

  4. One quick observation: if the user is not allowed to skip lines, why do we need this multi-line editor field? After filling my invoice description, I noticed that I cannot skip lines. It would be better if I got the error message before clicking "save" on "Edit description" modal. What do you think?

if (location.pathname !== to) {
history.push({ pathname: to, search: location.search });
}
}, [history, location.pathname, to]);
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting this warning about the location.search dep:

React Hook useEffect has a missing dependency: 'location.search'. Either include it or remove the dependency array

Copy link
Member

Choose a reason for hiding this comment

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

I see that as well.

@alexlyp
Copy link
Member

alexlyp commented Jan 13, 2020

I'm getting an error TypeError: supportedChars is undefined when going to the Account page.

TypeError: supportedChars is undefined buildUsernameRegex src/github.com/decred/politeiagui/src/containers/User/Detail/validation.js:23

@fernandoabolafio
Copy link
Member Author

@victorgcramos regarding your comments:

  1. The mobile view needs some left padding for dropdown menu options, see:

I centralized the content and the dropdown should be fine now.

  1. so, on mobile view, at least for Safari 13, I was not able to edit the subdomain field. All other fields are okay.

Let's create an issue for that aside with some other UX improvements for the submission form.
I don't want to keep growing this PR indefinitely for UX tweaks.

  1. The manage user tab should be only available for admins.

Why? This tab shows a different view for non-admins (fields aren't editable), so the user can inspect his own attributes for DCC.

  1. One quick observation: if the user is not allowed to skip lines, why do we need this multi-line editor field? After filling my invoice description, I noticed that I cannot skip lines. It would be better if I got the error message before clicking "save" on "Edit description" modal. What do you think?

Good catch! I replaced the multiline editor by a simple text input so it shouldn't be a problem anymore.

@fernandoabolafio
Copy link
Member Author

@alexlyp regarding

I'm getting an error TypeError: supportedChars is undefined when going to the Account page.

TypeError: supportedChars is undefined buildUsernameRegex src/github.com/decred/politeiagui/src/containers/User/Detail/validation.js:23

It was happening before the changes applied by decred/politeia#1049. Are you running politeia with this commit included?

@victorgcramos
Copy link
Member

victorgcramos commented Jan 14, 2020

Why? This tab shows a different view for non-admins (fields aren't editable), so the user can inspect his own attributes for DCC.

@fernandoabolafio I just said that because of the specs from #1470. Nvm!

@alexlyp
Copy link
Member

alexlyp commented Jan 14, 2020

It was happening before the changes applied by decred/politeia#1049. Are you running politeia with this commit included?

Yah looks like I was running an older version of piwww, all good now. @fernandoabolafio

@tiagoalvesdulce
Copy link
Member

All good here. Great job!

@alexlyp
Copy link
Member

alexlyp commented Jan 14, 2020

Looks good to me as well!

@fernandoabolafio I suppose we can merge this then @victorgcramos can start to work on integrating the recent invoice work (which is up for PR) and the DCC work as well. But if you have any ideas to ease that process I'm interested to hear your ideas.

@fernandoabolafio
Copy link
Member Author

@alexlyp I think that's the general plan. Merge this and then add all the other features.
I think is perfect that @victorgcramos works with the features he has been working already. Which is mostly the new things we're adding. @victorgcramos can you organize the work to be done and create issues for that? Such as Invoice improvements and DCC?

And the others can work on adding the existent features that I couldn't add yet. Such as payouts, invoice editing and so on. I created issues for those.

At some point, we will need to halt the work in the "old code" to be able to deliver the new web-app. I would rather do it sooner than later. What do you think?

@fernandoabolafio
Copy link
Member Author

Let's move this discussion to #1685.

@fernandoabolafio fernandoabolafio merged commit f0f6d0b into decred:cmsv2 Jan 15, 2020
thi4go pushed a commit to thi4go/politeiagui that referenced this pull request Jan 16, 2020
This PR sets the base layer for the new CMS web client. This PR adds the requirements for Signup, invoice submission and evaluation as well comments. This also adds the complementary features for a useful version of CMS such as invoice listing, account management, etc.
thi4go pushed a commit to thi4go/politeiagui that referenced this pull request Jan 16, 2020
This PR sets the base layer for the new CMS web client. This PR adds the requirements for Signup, invoice submission and evaluation as well comments. This also adds the complementary features for a useful version of CMS such as invoice listing, account management, etc.
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.

None yet

5 participants