Fixes #20995: Reject components from the same CV #6961
Conversation
|
Issues: #20995 |
spec/models/content_view_spec.rb
Outdated
| {content_view_version_id: version2.id}] } | ||
| let(:version1) { content_view_version.new(id = 3, content_view_id = 1) } | ||
| let(:version2) { content_view_version.new(id = 4, content_view_id = 1) } | ||
| let(:version3) { content_view_version.new(id = 5, content_view_id = 2) } |
There was a problem hiding this comment.
Useless assignment to variable - id.
Useless assignment to variable - content_view_id.
spec/models/content_view_spec.rb
Outdated
| let(:components) { [{content_view_version_id: version1.id}, | ||
| {content_view_version_id: version2.id}] } | ||
| let(:version1) { content_view_version.new(id = 3, content_view_id = 1) } | ||
| let(:version2) { content_view_version.new(id = 4, content_view_id = 1) } |
There was a problem hiding this comment.
Useless assignment to variable - id.
Useless assignment to variable - content_view_id.
spec/models/content_view_spec.rb
Outdated
| let(:content_view_version) { Struct.new(:id, :content_view_id) } | ||
| let(:components) { [{content_view_version_id: version1.id}, | ||
| {content_view_version_id: version2.id}] } | ||
| let(:version1) { content_view_version.new(id = 3, content_view_id = 1) } |
There was a problem hiding this comment.
Useless assignment to variable - id.
Useless assignment to variable - content_view_id.
spec/models/content_view_spec.rb
Outdated
| describe "#check_component_content_views_unique" do | ||
| let(:content_view_version) { Struct.new(:id, :content_view_id) } | ||
| let(:components) { [{content_view_version_id: version1.id}, | ||
| {content_view_version_id: version2.id}] } |
There was a problem hiding this comment.
Expression at 12, 69 should be on its own line.
spec/models/content_view_spec.rb
Outdated
|
|
||
| describe "#check_component_content_views_unique" do | ||
| let(:content_view_version) { Struct.new(:id, :content_view_id) } | ||
| let(:components) { [{content_view_version_id: version1.id}, |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
Block body expression is on the same line as the block start.
spec/models/content_view_spec.rb
Outdated
| describe ContentView do | ||
| describe "validation" do | ||
| let(:content_view) { ContentView.new } | ||
| let(:composite_content_view) { content_view.composite = true; content_view } |
There was a problem hiding this comment.
Do not use semicolons to terminate expressions.
spec/models/content_view_spec.rb
Outdated
| {content_view_version_id: version2.id}] } | ||
| let(:version1) { content_view_version.new(id = 3, content_view_id = 1) } | ||
| let(:version2) { content_view_version.new(id = 4, content_view_id = 1) } | ||
| let(:version3) { content_view_version.new(id = 5, content_view_id = 2) } |
There was a problem hiding this comment.
Useless assignment to variable - id.
Useless assignment to variable - content_view_id.
spec/models/content_view_spec.rb
Outdated
| let(:components) { [{content_view_version_id: version1.id}, | ||
| {content_view_version_id: version2.id}] } | ||
| let(:version1) { content_view_version.new(id = 3, content_view_id = 1) } | ||
| let(:version2) { content_view_version.new(id = 4, content_view_id = 1) } |
There was a problem hiding this comment.
Useless assignment to variable - id.
Useless assignment to variable - content_view_id.
spec/models/content_view_spec.rb
Outdated
| let(:content_view_version) { Struct.new(:id, :content_view_id) } | ||
| let(:components) { [{content_view_version_id: version1.id}, | ||
| {content_view_version_id: version2.id}] } | ||
| let(:version1) { content_view_version.new(id = 3, content_view_id = 1) } |
There was a problem hiding this comment.
Useless assignment to variable - id.
Useless assignment to variable - content_view_id.
spec/models/content_view_spec.rb
Outdated
| describe "#check_component_content_views_unique" do | ||
| let(:content_view_version) { Struct.new(:id, :content_view_id) } | ||
| let(:components) { [{content_view_version_id: version1.id}, | ||
| {content_view_version_id: version2.id}] } |
There was a problem hiding this comment.
Expression at 12, 69 should be on its own line.
spec/models/content_view_spec.rb
Outdated
|
|
||
| describe "#check_component_content_views_unique" do | ||
| let(:content_view_version) { Struct.new(:id, :content_view_id) } | ||
| let(:components) { [{content_view_version_id: version1.id}, |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
Block body expression is on the same line as the block start.
spec/models/content_view_spec.rb
Outdated
| describe ContentView do | ||
| describe "validation" do | ||
| let(:content_view) { ContentView.new } | ||
| let(:composite_content_view) { content_view.composite = true; content_view } |
There was a problem hiding this comment.
Do not use semicolons to terminate expressions.
|
@akofink as per our earlier discussion I am wondering if this logic is better added in content view component instead of ContentView model itself. The reason I say this is because this validation is about the relationship between 2 content views. Can you figure out why this did not get triggered |
|
More over. This call should ve failed on master https://github.com/Katello/katello/blob/master/app/models/katello/content_view.rb#L104 |
|
@parthaa I fixed the content view component validation. I'm still not convinced the component is the best place to put this validation, but it does work. One thing to note, when the error occurs, you always receive two error messages because there are two components with the same content view: Also, as you can see, the error message is generic rather than the expected message of |
Validate that components being added to a composite content view are versions of distinct content views
|
@parthaa I'm fine with merging as is after your review |
|
@akofink ack to this. I am totally ok merging this. But if you have reservations.
|
|
Let's merge as is. |
Validate that components being added to a composite content view are
versions of distinct content views