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

[WIP] Feature/profile page - requests for comments #34

Open
wants to merge 18 commits into
base: master
from

Conversation

@ccoenen
Copy link
Member

ccoenen commented Apr 4, 2019

(moved over from hackmdio/codimd#952 )

I am currently implementing a profile page that would let you edit your display name (henceforth: name) and possibly edit your password. It is far from finished, and I did a little bit of groundwork for future pages along the way. Those should get their own pull requests, probably.)

Questions:

  • Does EJS have a way to detect used but unset variables (like a strict mode for EJS, perhaps)?
  • We could put "delete user" on that page instead of the index-page-menu. Opinions? I think it's decided that it should go into profile.

TODO:

  • The header contains everything and the kitchen sink from the note editor. This needs to be fixed
  • user name will have to be sanitized. I would hate to restrict it to [a-z] (even though this would strictly not be a regression, it does feel like one). I have no experience with express/sequelize/ejs, so I would value your input - even just in form of documentation or a how-to, so I don't introduce XSS or something like that.
  • the password change is not implemented at all.
  • make sure I didn't mess up any of the login providers along the way (i.e. disable password boxes for ldap or github ...)
  • migration should pre-fill username / displayname fields for existing users

-----8<---- MVP line, cut here if neccessary ---8<----

  • (rudimentary) visual styling (right now, this is intended for code critique and conceptual work. Styling will be done later, of course.)
  • translations should be added to the correct jsons
  • username + displayname (display name defaulting to username, username defaulting to whatever it would have been so far. Perhaps the profile json will no longer be neccessary)
  • have /user/:username actually use username instead of user.id
  • documents i contributed and document I own is a bit tricky, those should be opt-in since they make documents discoverable that would otherwise have remained hidden. (checked off for "won't be done for now")
  • move "delete" and "export" functionality into user profile

For a MVP we could "just not link to it, yet" but already deploy it. We could get some feedback this way and improve it more before we officially ship it.

@ccoenen ccoenen self-assigned this Apr 4, 2019
@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Apr 4, 2019

(original comment by @SISheogorath hackmdio/codimd#952 (comment) )

We could put "delete user" on that page instead of the index-page-menu. Opinions?

Definitely! It was only put into the menu on the main page, because there was no profile page yet.

Did I get all the places, where those variables are used? Does EJS have a way to detect used but unset variables (like a strict mode for EJS, perhaps)?

Not sure if you got all, but EJS will definitely complain when there is something missing.


All in all, I really like your work! Seems like user profiles are moving forward! And I'm really curious about what we will get as result!

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Apr 4, 2019

(original comment by @davidmehren hackmdio/codimd#952 (comment) )

How does this interact with users from other auth sources, eg. LDAP? Normal apps might not have permission to change user attributes or delete them.

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Apr 4, 2019

(original comment by @SISheogorath hackmdio/codimd#952 (comment) )

@davidmehren right now, we delete all user data that exist in CodiMD's database when someone requests an account deletion. This means they can definitely re-auth with their external credentials afterwards, but they start from scratch, like their account never existed before.

Just realized you referred to the editing of the attributes in CodiMD. Not sure if we want to restrict them. How do other application handle that? I think most of them simply allow to change these details. At least discourse and GitLab do so, as far as I can say.

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Apr 4, 2019

(my comment originally posted here: hackmdio/codimd#952 (comment) )

Thanks for reminding me, until now i was only concerned with local users. I will add it to the TODO list. Since I use none of the other login providers, I'd value any input on that part. I might need to disable password change for those users, but display name (name attribute) ist completely local in any case. It's what will be listed in the "2 ONLINE" button top right, for example.

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Apr 4, 2019

unresolved debate about length of user names can be found here: hackmdio/codimd#952 (review)

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Apr 4, 2019

(my comment originally published here: hackmdio/codimd#952 (comment) )

I will be going for username / displayname next. Along the way I'll probably rebase this a few times, so previous discussion might end up folded up in github.

@ccoenen ccoenen force-pushed the ccoenen:feature/profile-page branch from 07db983 to 1b23905 Apr 4, 2019
@ccoenen ccoenen mentioned this pull request Apr 4, 2019
6 of 9 tasks complete
@ccoenen ccoenen force-pushed the ccoenen:feature/profile-page branch from 1b23905 to 4dcdbb1 Apr 4, 2019
@HerHde

This comment has been minimized.

Copy link
Contributor

HerHde commented Apr 5, 2019

I would rather set a minimum length of 1 for display names. But there are reasons to not limit anything ;-)
And why should we set a minimum of 3 chars for login-/usernames?

@ccoenen ccoenen changed the title [WIP] Feature/profile page - requests for comments WIP Feature/profile page - requests for comments Apr 8, 2019
@ccoenen ccoenen force-pushed the ccoenen:feature/profile-page branch 4 times, most recently from ba2b91b to 347018c May 11, 2019
@cyroxx

This comment has been minimized.

Copy link

cyroxx commented Aug 4, 2019

Is there anything that can be done to move things forward on this one?

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Aug 4, 2019

I rebase this on top of the current master from time to time. I will not be able to do any meaningful work for the next four or five weeks on this, I'm afraid. But: anyone who wants to work on this is very welcome to do so! I'm on riot at least once a day and I'll gladly answer any questions there or in this pull request!

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Aug 4, 2019

What would help me tremendously is testing the other login providers. I only use email login, so I haven't really tested yet if I interfered with the others in any way so far.

@ccoenen ccoenen mentioned this pull request Aug 26, 2019
ccoenen added 9 commits Sep 9, 2018
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
ccoenen added 2 commits Nov 4, 2018
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
@ccoenen ccoenen changed the title WIP Feature/profile page - requests for comments [WIP] Feature/profile page - requests for comments Nov 26, 2019
@ccoenen ccoenen force-pushed the ccoenen:feature/profile-page branch from 347018c to 91a87f3 Nov 26, 2019
ccoenen added 7 commits Nov 7, 2018
Signed-off-by: Claudius <opensource@amenthes.de>
…se you couldn't add a new user with the current signup form

Signed-off-by: Claudius <opensource@amenthes.de>
Deleting a user from your account management page makes more sense.
Also, so far this action was triggered by a GET request, now it's
a more reasonable POST request, which is the suggested method for
destructive things.

Features page is now more prominently linked to from the history or
landing page. It was previously below the user dropdown.

Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
Signed-off-by: Claudius <opensource@amenthes.de>
@ccoenen ccoenen force-pushed the ccoenen:feature/profile-page branch from 91a87f3 to e8295d9 Nov 26, 2019
@SISheogorath SISheogorath added this to the Release 2.0 milestone Feb 24, 2020
@SISheogorath

This comment has been minimized.

Copy link
Member

SISheogorath commented Feb 24, 2020

@ccoenen Want to rebase to 2.0?

@ccoenen

This comment has been minimized.

Copy link
Member Author

ccoenen commented Feb 25, 2020

Yes, I'll do this. I think we should finish this one for 2.0 too. I will need some help with testing, but other than that this shouldn't be very problematic.

I can start working on that in a week or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.