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

KG - OS Setting Fixes #1014

Merged
merged 6 commits into from Jul 26, 2017

Conversation

kayla-glick
Copy link
Contributor

@kayla-glick kayla-glick commented Jul 19, 2017

  1. The #value method on the Setting model was missing a default return. It returned nil unless the setting was a JSON or boolean type.
  2. The value validation on the Setting model incorrectly used false as a default, but should use true. This prevents errors from being added for string type settings.
  3. The rake task entered booleans as 't' and 'f' instead of 'true' and 'false'.
  4. Added a general friendly_name and description to each setting in the rake task.
  5. Added missing types email, url, and path to rake task
  6. Fixed url regex. Was incorrectly using . which matched any character instead of \. which matches only the . character. Also allows more flexible strings in the domain.
  7. Updated path and URL regexes to be stricter and more complete

Copy link
Contributor

@SamWord SamWord left a comment

Choose a reason for hiding this comment

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

See comment on regex

@@ -39,7 +39,7 @@ def is_email?(value)
end

def is_url?(value)
value.match?(/^((ftp|http|https):\/\/)?[a-zA-Z0-9]+(.[a-zA-Z0-9])?(:[0-9]+)?/)
value.match?(/^((ftp|http|https):\/\/)?[a-zA-Z0-9]+((\.[a-zA-Z0-9]+)|(:[0-9]+)+)/)
Copy link
Contributor

Choose a reason for hiding this comment

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

This regex doesn't allow for multiple periods in a url, and it allows more than one port. I'm not sure how big of an issue it is, but 123.0.0.0:3000 would not match, and localhost:3000:3000 would match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'm fixing this part of the URL and adding an optional path regex after it (updated from the current path regex) to ensure that the URL is fully valid.

@kayla-glick
Copy link
Contributor Author

@Stuart-Johnson
The test suite is unable to run currently on os-move-configuration-to-database so this branch won't be able to pass on Travis.

@sparc-request
Copy link
Owner

sparc-request commented Jul 21, 2017 via email

@kayla-glick
Copy link
Contributor Author

kayla-glick commented Jul 21, 2017

@sparc-request
This branch actually already has the Travis.yml updates. os-move-configuration-to-database is having an issue finding a Setting in the rspec setup (ldap.rb:21) that needs to be resolved at some point.

@kayla-glick kayla-glick merged commit b1f01e3 into os-move-configuration-to-database Jul 26, 2017
@kayla-glick kayla-glick deleted the os-settings_updates branch July 26, 2017 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants