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

username/display_name/video_channel_name min length 1 and max length 50 #1265

Merged
merged 4 commits into from
Dec 7, 2018

Conversation

McFlat
Copy link
Contributor

@McFlat McFlat commented Oct 14, 2018

! still some bug on the frontend complains but if you remove the disabled property it creates the account just fine (fixes #1263)

@McFlat McFlat force-pushed the feature/longer-usernames branch from 2fa0e44 to 29faa6c Compare October 14, 2018 00:18
@McFlat McFlat force-pushed the feature/longer-usernames branch from 29faa6c to 3ecd383 Compare October 15, 2018 05:58
@McFlat McFlat changed the title make username, display_name and video_channel_name min length 1 and … username/display_name/video_channel_name min length 1 and max length 50 Oct 15, 2018
@McFlat McFlat force-pushed the feature/longer-usernames branch 6 times, most recently from 89f6121 to 2b46153 Compare October 15, 2018 19:18
@rigelk
Copy link
Collaborator

rigelk commented Oct 15, 2018

Do you still have the frontend bug?

@McFlat
Copy link
Contributor Author

McFlat commented Oct 16, 2018

@rigelk yes, not sure if it's something on my end only, but it still complains about username being less than 3 or more than 20 chars, but can "Inspect" the elements and remove the disabled field from the submit button and it works fine.

I've looked all over the codebase to see what else could be causing it but can't seem to find anything else, pretty sure I changed all places that apply.

Could it be the locales?

@McFlat
Copy link
Contributor Author

McFlat commented Oct 16, 2018

We have this in the client code now:

    this.USER_USERNAME = {
      VALIDATORS: [
        Validators.required,
        Validators.minLength(1),
        Validators.maxLength(50),
        Validators.pattern(/^[a-zA-Z0-9]+[a-zA-Z0-9-._]+$/)
      ],
      MESSAGES: {
        'required': this.i18n('Username is required.'),
        'minlength': this.i18n('Username must be at least 1 character long.'),
        'maxlength': this.i18n('Username cannot be more than 50 characters long.'),
        'pattern': this.i18n('Username should start with a letter or number; dots, dashes and underscores are allowed.')
      }
    }

But in the frontend, when I run it, I see this validation error:
screen shot 2018-10-15 at 8 09 35 pm
screen shot 2018-10-15 at 8 10 48 pm

However I can remove the disabled attribute added to the submit/signup button and it all works fine, it creates the account.
screen shot 2018-10-15 at 8 11 36 pm

@McFlat
Copy link
Contributor Author

McFlat commented Oct 16, 2018

Could possibly be a bug in the validator, unless it's using the locales somehow to do the validation, which doesn't make sense but that could be part of the bug.

@McFlat
Copy link
Contributor Author

McFlat commented Oct 16, 2018

We should probably add some better tests for this buggy issue.

@rigelk rigelk added the Status: Requires Tests Either requires manual test, or writing tests, or both label Oct 16, 2018
Copy link
Collaborator

@rigelk rigelk left a comment

Choose a reason for hiding this comment

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

I can't reproduce your bug with a development environment: yarn run dev. Please review your regex too.

@McFlat
Copy link
Contributor Author

McFlat commented Oct 16, 2018

Wait so you can create a username with a single letter or a username with more than 20 chars long without seeing the validation errors? if so then for sure it's only with my environment

@McFlat
Copy link
Contributor Author

McFlat commented Oct 16, 2018

I've been using node 9.11.2 while working on this, I'll have to check with node version 8.x to see if it behaves differently.

Update: It does the same thing with node 8.12.0, I'll have to clone the project in a separate directory to see how it behaves next.

Update: had to fix the regex to be /^[a-zA-Z0-9][a-zA-Z0-9-._]*$/ because before it would match 2 chars, but the validation error still is an issue on my end, like before.

Update: there's some serious bug in the frontend validator, the regular expression specifically doesn't allow a username to start with., - or _ but here the validator doesn't flag error for a username with 3 dots or 3 underscores for that matter.

screen shot 2018-10-16 at 2 12 08 pm

and if I enable global multiline in the regexr.com while testing the regex, the regex notation is valid, so it must be some major bug with the validator and the way it parses the regex perhaps.

screen shot 2018-10-16 at 2 16 05 pm

Update: ok so turns out it has to do with the way we are building the forms with the formBuilder/FormValidatorService. I think we need to be doing things a little bit differently here, seems to me the angular docs had a minlength and maxlength validator but we don't use them, also the docs have a regex validator but it looks like we do things a little differently than how the docs say to do them, maybe we are doing them this way to cut corners to make things more dynamic, idk but whatever it is there's bugs in this functionality, at least on my end there are, I still haven't figured out why another person could have different results than me, if anything it's possibly just my environment. On the other side, in the server we don't have any kind of validation, only min and max limits, which means someone with a set of crafty ideas can really abuse the service if they know how, by disabling the stuff on the frontend like I was doing to submit the form and the backend will allow it. Really what needs to happen is the same regex and limits need to be on the client and the server without any exceptions.

Update: I figured out what the problem was, I was accessing the site on port 9000, while I should have been accessing it on port 3000, after this all those validation errors went away. Now I can get back to getting this finished up, finally!

@McFlat McFlat force-pushed the feature/longer-usernames branch from 2b46153 to 64779bb Compare October 16, 2018 19:06
Copy link
Owner

@Chocobozzz Chocobozzz left a comment

Choose a reason for hiding this comment

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

Please test video channels name too (check-params/video-channels.ts)

@McFlat McFlat force-pushed the feature/longer-usernames branch from 64779bb to 97e720a Compare October 17, 2018 16:01
@McFlat
Copy link
Contributor Author

McFlat commented Oct 17, 2018

All my tests seem to work all good, now that I figured out port 3000 not port 9000. Everything works great, I think this is ready to be merged unless you guys want to add something else like more tests that check the different variations of username or video channel names that can be created.

@McFlat McFlat force-pushed the feature/longer-usernames branch from 97e720a to ebaad2a Compare October 18, 2018 02:16
@McFlat
Copy link
Contributor Author

McFlat commented Oct 18, 2018

I had to amend/modify the commit message(remove a semicolon) and push force to make it pass these tests that have nothing to do with what this pull request fixes. If it passes this time, and you guys agree with the changes then please merge. Keep up the great work guys, lots of people need this platform.

screen shot 2018-10-17 at 9 17 01 pm

screen shot 2018-10-17 at 9 17 49 pm

…ax length 50; (fixes Chocobozzz#1263);

    ! still some bug on the frontend complains but if you remove the disabled property it creates the account just fine;
allow for usernames to start with a number;
fix test, since username can be 1 char now make test check empty;
fix test, Should fail with a too long username;
fix test, Should fail with a too small username;
fix regular expression for username and videoChannel;
change username, videoChannel to be lowercase and fix message;
@McFlat McFlat force-pushed the feature/longer-usernames branch from 9ebe1d3 to 11d86c3 Compare October 18, 2018 03:27
@Chocobozzz
Copy link
Owner

Should be fixed if you rebase develop

@McFlat
Copy link
Contributor Author

McFlat commented Oct 19, 2018

I'll fix the merge conflict right now

@McFlat McFlat force-pushed the feature/longer-usernames branch from 669ae92 to 4c8aff2 Compare October 19, 2018 12:37
@McFlat
Copy link
Contributor Author

McFlat commented Oct 20, 2018

Can you guys still see this pull request? My account got flagged again, github can lick my gonadz I wasn't even doing anything this time, just creating a gist note for myself, bastards, can't even work on github

@McFlat
Copy link
Contributor Author

McFlat commented Oct 20, 2018

@Chocobozzz @rigelk If you can please merge this it would be great otherwise I'll have to create another account and re push this these changes on a different pull request. My account keeps getting flagged by github and they tell me to contact the owner of this repo, then I look crazy, but reality is they are a bunch of clowns. I'm starting to really hate github after loving it so much for over a decade.

Validators.pattern(/^[a-z0-9._]+$/)
Validators.minLength(1),
Validators.maxLength(50),
Validators.pattern(/^[a-z0-9][a-z0-9-._]*$/)
Copy link
Owner

Choose a reason for hiding this comment

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

- char is not accepted by mastodon i think

Copy link
Contributor Author

@McFlat McFlat Nov 25, 2018

Choose a reason for hiding this comment

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

I'll have to fix that, or maybe we can have mastodon accept it?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is two issues at least:

but that doesn't seems to be planned for the moment..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me it seems that mastodon accepts everything accept the . char, which it removes if it sees it.

https://github.com/tootsuite/mastodon/blob/master/app/validators/unique_username_validator.rb#L7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screen shot 2018-11-25 at 6 04 44 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screen shot 2018-11-25 at 6 06 07 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it's safe to merge the pull request without any other changes to it

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still not safe, see my previous comment.
You are only showing basic HTML5 validation (field filled or not).
Validate the form and it will refuse it:
image

There is two issues still opened, and Gargron stated three days ago that dashes are not accepted from remote usernames.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a scenario where a user can have spaces in their very long username, even beyond the 50 chars limit we have with this pull request. very-long username-a user likes just because he likes it
screen shot 2018-11-25 at 6 14 14 am

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you didn't submitted the form, the HTML5 shows a green border only because the field is filled and not empty (because of the "required" html attribute).

@McFlat
Copy link
Contributor Author

McFlat commented Nov 25, 2018

This pull request is ready for merge, the buggy issues I was having was due to my environment, specifically with accessing the app on a unrecommended port, other than that everything looks good. I also checked the username with mastodon and it seems to work fine without issues, we are compatible with mastodon. Mastodon actually accepts spaces in their usernames too, but I don't think we should support spaces, just a longer length and dots, dashes, underscores is good enough.

@rhaamo
Copy link
Contributor

rhaamo commented Nov 25, 2018

I also checked the username with mastodon and it seems to work fine without issues, we are compatible with mastodon. Mastodon actually accepts spaces in their usernames too, but I don't think we should support spaces, just a longer length and dots, dashes, underscores is good enough.

Please see my previous comments, mastodon accepts only : letters, numbers and underscores

Validate the register form and you will see it yourself:
image

@McFlat
Copy link
Contributor Author

McFlat commented Nov 25, 2018

@rhaamo good point, yeah I never submitted it, 🤦‍♂️ I can see your comments now, I didn't see them before

Also I was wrong about the length, they limit the username length at 30 chars

https://github.com/tootsuite/mastodon/blob/d6b9a62e0a13497b8cc40eb1db56d1ec2b3760ea/spec/models/account_spec.rb#L587

I think it's too limiting on their end, we should create an issue and a pull request to fix that in their repo. They should also fix the frontend validation rules to not waste users time, in case when their take so long to pick a username and see all green to go then to just get denied and have to spend more time to pick their username within the limits.

I've submitted an issue on the mastodon repo about this mastodon/mastodon#9346

Thanks @rhaamo I wasn't aware of that, I was away from this project for a month or so, not that I'm back yet, just wanted to do a checkup on the issues and comments etc. just been busy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Requires Tests Either requires manual test, or writing tests, or both
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for username max length to be 50 chars instead of 20, 20 is too limiting
4 participants