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

Fix for #3722 #4097

Merged
merged 6 commits into from
Aug 3, 2016
Merged

Fix for #3722 #4097

merged 6 commits into from
Aug 3, 2016

Conversation

kaguillera
Copy link
Contributor

@kaguillera kaguillera commented Jul 21, 2016

This should fix the issue #3722

Todo

@kaguillera
Copy link
Contributor Author

@whit537 is this good? Let me know what you think.

@@ -3,14 +3,14 @@
from postgres.orm import Model


name_pattern = re.compile(r'^[A-Za-z0-9,._ -]+$')
name_pattern = re.compile(r"^[A-Za-z0-9,._ '-]+$")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should modify the semantics of the code we have in production. I think instead we should modify fake data to not use community names that don't pass name_pattern.

@kaguillera
Copy link
Contributor Author

kaguillera commented Aug 3, 2016

I made a boo boo again by pushing a large number of commits that have nothing to do with this PR or even the branch that I am working on.

@kaguillera kaguillera force-pushed the fix-fakename-trip-up branch from aaf7a5f to 65274e9 Compare August 3, 2016 19:40
@kaguillera
Copy link
Contributor Author

Ready for review again @whit537

@@ -3,7 +3,6 @@
from gratipay.utils import fake_data
from gratipay.testing import Harness


Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put this line back. No reason to remove it on this PR.

"""
crusher = self.make_participant('crusher', email_address='[email protected]')
team = fake_data.fake_team( self.db, crusher, "D'Amorebury")
assert team.name != "d-amorebury"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ... is this the best assertion here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it isn't but the alternative I can't really see

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if the test is called test_fake_team_does_not_fail_for_teams_with_apostrophes then the assertion is sufficient. :)

@chadwhitacre
Copy link
Contributor

Pass done. Todo in the description!

@chadwhitacre
Copy link
Contributor

@kaguillera Reviewed latest commits, updated Todo. ;)

@chadwhitacre chadwhitacre merged commit 7db4d94 into master Aug 3, 2016
@chadwhitacre chadwhitacre deleted the fix-fakename-trip-up branch August 3, 2016 20:54
@chadwhitacre chadwhitacre mentioned this pull request Aug 3, 2016
15 tasks
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.

2 participants