Skip to content

Conversation

t8y8
Copy link
Collaborator

@t8y8 t8y8 commented Oct 18, 2016

Content_urls can only be alphanumeric or underscores and dashes. Fix is to validate it client side because the Server error is confusing and the client should just prevent invalid urls from going through.

  • Added a generic regex validator for properties and apply it to the site_model
  • Updated site_item_tests with some unicode and valid/invalid cases

Copy link
Contributor

@graysonarts graysonarts left a comment

Choose a reason for hiding this comment

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

we need the message to be configurable for potential reuse.


def property_matches(regex_to_match):

COMPILED_RE = re.compile(regex_to_match)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a constant, so it should be compiled_re

@wraps(func)
def validate_regex_decorator(self, value):
if not COMPILED_RE.match(value):
raise ValueError("content_url can contain only ascii letters, numbers, dashes, and underscores")
Copy link
Contributor

Choose a reason for hiding this comment

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

Make the message an argument that gets passed in because if we want to reuse this, then the error message would be wrong.

from .. import NAMESPACE


VALID_CONTENT_URL_RE = 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.

Do we ever want the content url to be empty? I guess it is when we have the default site, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, it needs to validate "" or all of our models fail when reading responses from the default site

@@ -1,3 +1,5 @@
# coding=utf-8
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be part of our template for new files!

Copy link
Contributor

@graysonarts graysonarts left a comment

Choose a reason for hiding this comment

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

🚀

@t8y8 t8y8 merged commit 4f4112b into tableau:development Oct 18, 2016
@t8y8 t8y8 deleted the 50-validate-content-url branch October 18, 2016 21:18
bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request Dec 28, 2023
…leau#64)

* Bump test coverage up a bit in fields
* Fix pep8 violation, and it fixed a few other things automatically as well
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.

2 participants