Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Team name restrictions #3943

Merged
merged 11 commits into from
Mar 15, 2016
Merged

Team name restrictions #3943

merged 11 commits into from
Mar 15, 2016

Conversation

aandis
Copy link
Contributor

@aandis aandis commented Mar 6, 2016

@aandis
Copy link
Contributor Author

aandis commented Mar 7, 2016

@mattbk this look good?

@@ -71,6 +71,11 @@ if request.method == 'POST':
))

try:
fields['slug'] = slugize(fields['name'])
except AssertionError:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd make more sense to throw a custom error - InvalidTeamName, maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wokay! It did cross my mind too but it looked like a bit too much. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another reason I didn't do this is that slugize is from community model. So having Invalid*Team*Name in there wouldn't really fit well. Now that you mentioned #3426, this would make sense since team model would have a slugize of its own.

@mattbk
Copy link
Contributor

mattbk commented Mar 7, 2016

I can't test right now, but looks like you're on the right track.

@mattbk
Copy link
Contributor

mattbk commented Mar 7, 2016

It's funny that you can't use these characters when creating a new team, but you can edit your team name to include any characters you want.

@mattbk
Copy link
Contributor

mattbk commented Mar 8, 2016

Hmm, my error messages are stacking up:
5df1d3b3-04d8-4615-bf12-58c5afee520c

@mattbk
Copy link
Contributor

mattbk commented Mar 8, 2016

A team name of all underscores collapses into a team with a name of length zero, and the form completes. The team ends up under Review, but the link goes nowhere.

It looks like all underscores and periods collapse into "-" if they are between alphanumerics. In that case, I wouldn't include them in the list of approved characters?

@rohitpaulk
Copy link
Contributor

It looks like all underscores and periods collapse into "-"

Side-effect of #3426, looks like

@aandis
Copy link
Contributor Author

aandis commented Mar 8, 2016

you can't use these characters when creating a new team, but you can edit your team name to include any characters you want.

There's a reason for this. You can't include those characters while creating the team because we use those characters to create a (unique) slug for the team and the current method which does that expects only certain characters. But when editing the team, the slug is already created and you can't really change it, so you should be free to edit your team name however you want.

@mattbk
Copy link
Contributor

mattbk commented Mar 8, 2016

Okay, thanks, that makes sense.

@aandis
Copy link
Contributor Author

aandis commented Mar 9, 2016

Alright. Fixed #3880 and #3426 in this pr. Ready for review.

@aandis
Copy link
Contributor Author

aandis commented Mar 10, 2016

I added the team name regex such that things like #3943 (comment) can be avoided.

@mattbk
Copy link
Contributor

mattbk commented Mar 14, 2016

It works for me. I'm happy with it as long as someone else thinks the new slugize works right. @whit537 @rohitpaulk ?

@chadwhitacre
Copy link
Contributor

Reviewing ...

@chadwhitacre
Copy link
Contributor

Rebased on master at af05120. Previous head was 8fa155e.

@@ -58,6 +57,7 @@ still_migrating = delta > 0
<input type="hidden" name="csrf_token" value="{{ csrf_token }}">

<label><h2>{{ _("Team Name") }}</h2></label>
<p><i>{{ _("Must contain alphabets. Can optionally contain numbers, dashes (-), underscores (_), periods (.), comma (,) and whitespaces.") }}</i></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this usage of "alphabets." Perhaps it's part of Indian English?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh. It's petty common over here. I just realized myself that it's wrong though. 😳

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries ... maybe we should have an Indian English translation! 💃

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, "localization" is probably the better word. ;-)

@chadwhitacre
Copy link
Contributor

Rebased to remove 3c0d72f in favor of cbe6dd7. Previous head was 48f9ae6.

@chadwhitacre
Copy link
Contributor

Rebasing again to fix Fixing a couple tests ...

@chadwhitacre
Copy link
Contributor

@aandis I'm ready to merge if you're good with my latest commits ...

@aandis
Copy link
Contributor Author

aandis commented Mar 15, 2016

Commented on 3c0d72f#commitcomment-16714701

@chadwhitacre
Copy link
Contributor

@aandis Back in 2196bd6. :)

@aandis
Copy link
Contributor Author

aandis commented Mar 15, 2016

Looks good to me. 👍

chadwhitacre added a commit that referenced this pull request Mar 15, 2016
@chadwhitacre chadwhitacre merged commit 169dc57 into master Mar 15, 2016
@chadwhitacre chadwhitacre deleted the team-name-restrictions branch March 15, 2016 18:16
@chadwhitacre
Copy link
Contributor

!m @aandis

@chadwhitacre chadwhitacre added this to the Team Basics milestone Mar 15, 2016
@aandis
Copy link
Contributor Author

aandis commented Mar 15, 2016

!m @whit537

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