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.

Sign up for GitHub

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

Jump to bottom

Fixes #24057 Email subject prefix accepts long strings #5742

Merged
merged 1 commit into from
Jun 28, 2018
Merged

Fixes #24057 Email subject prefix accepts long strings #5742

merged 1 commit into from
Jun 28, 2018

Conversation

apuntamb
Copy link
Member

No description provided.

@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

timogoebel
timogoebel requested changes Jun 26, 2018
View reviewed changes
Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

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

@apuntamb: Thanks. Please see my comments inline.

test/models/settings/email_setting_test.rb Outdated
@@ -22,6 +22,11 @@ class EmailSettingTest < ActiveSupport::TestCase
assert_equal({"address" => "string@example.com", "authentication" => 'plain'}, Setting::Email.delivery_settings)
end

test 'value of email_subject_prefix should not be more than 255 characters' do
Setting[:email_subject_prefix] = 'p' * 255
assert_not Setting::Email.new.valid?
Copy link
Member

Choose a reason for hiding this comment

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

We have a nice helper for this: refute_valid

app/models/setting.rb Outdated
@@ -50,6 +50,7 @@ def validate(record)
validates_with ValueValidator, :if => Proc.new {|s| s.respond_to?("validate_#{s.name}") }
validates :value, :array_hostnames => true, :if => Proc.new { |s| ARRAY_HOSTNAMES.include? s.name }
validates :value, :email => true, :if => Proc.new { |s| EMAIL_ATTRS.include? s.name }
validates :value, :length => {:maximum => 255}, :if => Proc.new { |s| s.name == "email_subject_prefix" }
Copy link
Member

Choose a reason for hiding this comment

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

Can we safely move this to app/models/setting/email.rb?

@apuntamb apuntamb force-pushed the validate_email_subject_prefix branch from 7a128ab to 6493d07 Compare June 26, 2018 12:16
@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • 6493d07 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

@apuntamb apuntamb force-pushed the validate_email_subject_prefix branch from 6493d07 to 4153848 Compare June 26, 2018 12:29
timogoebel
timogoebel previously requested changes Jun 26, 2018
View reviewed changes
test/models/settings/email_setting_test.rb Outdated
@@ -22,6 +22,11 @@ class EmailSettingTest < ActiveSupport::TestCase
assert_equal({"address" => "string@example.com", "authentication" => 'plain'}, Setting::Email.delivery_settings)
end

test 'value of email_subject_prefix should not be more than 255 characters' do
Setting[:email_subject_prefix] = 'p' * 255
refute_valid Setting::Email.new
Copy link
Member

Choose a reason for hiding this comment

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

I think the test now tests the wrong thing. This is probably invalid because no name is set. Can you please check if the setting you create above is not valid?

@apuntamb apuntamb force-pushed the validate_email_subject_prefix branch from 4153848 to 38dfc53 Compare June 28, 2018 09:12
dLobatog
dLobatog approved these changes Jun 28, 2018
View reviewed changes
Copy link
Member

@dLobatog dLobatog left a comment

Choose a reason for hiding this comment

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

Test looks like it was fixed, thanks @apuntamb @timogoebel

@dLobatog dLobatog dismissed timogoebel’s stale review June 28, 2018 16:51

fixed test - Setting::Email.new is not valid with a subject of 256 chars

@dLobatog dLobatog merged commit 87e2335 into theforeman:develop Jun 28, 2018
@lzap
Copy link
Member

lzap commented Jun 29, 2018 •
edited
Loading

This patch broke develop tests:

Error:
EmailSettingTest#test_0004_value of email_subject_prefix should not be more than 255 characters:
ActiveRecord::RecordInvalid: Validation failed: Value is too long (maximum is 255 characters)
    app/models/setting.rb:111:in `[]='
    test/models/settings/email_setting_test.rb:26:in `block in <class:EmailSettingTest>'

@apuntamb @dLobatog short-term plans for fix?

@lzap
Copy link
Member

lzap commented Jun 29, 2018

@dLobatog the reason for that was that Jenkins did not run Foreman tests at all...

@lzap lzap mentioned this pull request Jun 29, 2018
Merged
@apuntamb
Copy link
Member Author

apuntamb commented Jul 2, 2018

Thanks @lzap for the quick fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dLobatog dLobatog dLobatog approved these changes

@timogoebel timogoebel timogoebel left review comments

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@apuntamb @theforeman-bot @lzap @dLobatog @timogoebel