SLE15 product tree support -- 4702 #72

ikapelyukhin · 2018-01-03T14:18:09Z

No description provided.

ikapelyukhin · 2018-01-17T13:29:14Z

I want to add one more spec for the ProductSerializer, but the rest should be ready for the review.

suse-tests-pass · 2018-01-17T13:30:34Z

@lagartoflojo will review your pull request 👯 check https://trello.com/c/p2hEsVT1/4702-5-rmt-sles-15-product-tree-support
@lagartoflojo: Please review this pull request using our guidelines:

test for acceptance criteria / functionality
check if the new code is covered with tests
check for unintended consequences
encourage usage of the boyscout rule
make sure the code is architected in the best way
check that no unnecessary technical debt got introduced
make sure that no unnecessary FIXMEs or TODOs got added

ikapelyukhin · 2018-01-17T15:44:55Z

OK, that should be it.

lagartoflojo

Haven't tried it out locally yet. I'll do that now.

lagartoflojo · 2018-01-18T15:14:26Z

spec/models/product_spec.rb

+      level_two_extension_b_not_mirrored
+      level_two_extension_a_mirrored
+      level_two_extension_b_mirrored
+    end


This looks funny, why not use let!?

Because Rubocop complains about it:

spec/models/product_spec.rb:93:5: C: RSpec/LetSetup: Do not use let! for test setup. let!(:level_two_extension_a_not_mirrored) { FactoryGirl.create(:product, :extension, base_products: [level_one_extension], root_product: base_a) }

I guess using the before block makes the initialization order more explicit.
But I can disable the check if you think it doesn't make sense.

Hmm strange rule. It must be new. We use let! all over the place (even in RMT hehe)

It seems it doesn't complain about all of the let! calls, but only about the ones that aren't called from anywhere.

Here's the discussion about why it's useful.

I see. With that logic, they actual create calls should be moved into the before block then.

Moving into the create block would make it impossible to reference them later in the expect block.
A couple of those aren't referenced, so yes, those could be moved in, but I'd say it's not very important.

Ok, let's leave it as is then.

@thutterer ^^^

lagartoflojo · 2018-01-18T15:17:28Z

spec/serializers/v3/product_serializer_spec.rb

+
+    it 'has base system extension' do
+      expect(top_extension[:name]).to eq(basesystem.name)
+    end


These can be re-written as:
its([:name]) { is_expected.to eq basesystem.name }

Same for the tests below.

Not really, it's not the subject of the spec.

For the top level specs I'd need to do:

subject { described_class.new(sled15, root_product: sled15, base_url: base_url)[:extensions].first }

And for nested extensions I'd need to do:

subject { described_class.new(sled15, root_product: sled15, base_url: base_url)[:extensions].first[:extensions].first }

That doesn't seem to make anything better, though.

Ah, didn't catch that, you're right.

lagartoflojo · 2018-01-18T16:31:37Z

This PR is not working for me:

> ./bin/rmt-cli sync
I, [2018-01-18T17:26:45.890722 #24033]  INFO -- : Cleaning up the database
I, [2018-01-18T17:26:45.921413 #24033]  INFO -- : Downloading data from SCC
I, [2018-01-18T17:26:45.921473 #24033]  INFO -- : Updating products
I, [2018-01-18T17:28:13.144281 #24033]  INFO -- : Updating repositories
/home/hernan/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/activerecord-5.1.3/lib/active_record/core.rb:189:in `find': Couldn't find Repository with 'id'=1094 (ActiveRecord::RecordNotFound)
	from /code/rmt/lib/rmt/scc.rb:152:in `update_auth_token'
	from /code/rmt/lib/rmt/scc.rb:33:in `block in sync'
	from /code/rmt/lib/rmt/scc.rb:32:in `each'
	from /code/rmt/lib/rmt/scc.rb:32:in `sync'
	from /code/rmt/lib/rmt/cli/main.rb:11:in `sync'
	from /home/hernan/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/thor-0.20.0/lib/thor/command.rb:27:in `run'
	from /home/hernan/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in `invoke_command'
	from /home/hernan/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/thor-0.20.0/lib/thor.rb:387:in `dispatch'
	from /code/rmt/lib/rmt/cli/base.rb:22:in `block in dispatch'
	from /code/rmt/lib/rmt/cli/base.rb:33:in `handle_exceptions'
	from /code/rmt/lib/rmt/cli/base.rb:22:in `dispatch'
	from /home/hernan/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/thor-0.20.0/lib/thor/base.rb:466:in `start'
	from ./bin/rmt-cli:40:in `<main>'

The same command works fine in master.

lagartoflojo · 2018-01-18T16:36:07Z

app/serializers/v3/product_serializer.rb

-    object.mirrored_extensions.map do |extension|
-      ::V3::ProductSerializer.new(extension, base_url: base_url).attributes
+    object.extensions.for_root_product(root_product).map do |extension|
+      ::V3::ProductSerializer.new(extension, base_url: base_url, root_product: root_product).attributes
    end


In glue we simply have a has_many :extensions, and all the instance_options are passed down to the child serializers automatically, so you don't need to specify base_url or root_product.
That leaves you with:

has_many :extensions, serializer: V3::ProductSerializer def extensions object.extensions.for_root_product(root_product) end

I remember I was looking into this at the very beginning and the current code is the solution that works correctly.

The problem is that for some reason it doesn't serialize all of the attributes of the relation, namely the ones that are declared on the serializer and don't exist on the model (like available and recommended). With the has_many relation they seem to be just missing.

If you have any idea what's going on with that -- feel free to share.

Hmm weird, has_many also doesn't work for me. But because it doesn't output the nested extension... Anyway!

ikapelyukhin · 2018-01-22T15:45:49Z

@lagartoflojo, syncing should work without issues now.

lagartoflojo

one missing test, otherwise lgtm

lagartoflojo · 2018-01-23T11:29:42Z

app/models/product.rb

@@ -69,6 +77,10 @@ def change_repositories_mirroring!(conditions, mirroring_enabled)
    repositories.where(conditions).update_all(mirroring_enabled: mirroring_enabled)
  end

+  def recommended_for?(root_product)


I think we're missing unit tests for this

Yep, it was covered only by serializer specs. I've added a test for this method.

lagartoflojo

I left a refactoring for your spec, if you want to incorporate it. If not, still LGTM.

lagartoflojo · 2018-01-23T15:44:39Z

spec/models/product_spec.rb

+      extension = FactoryGirl.create(:product, :extension, base_products: [base_a], recommended: true)
+      expect(extension.recommended_for?(base_b)).to be(false)
+    end
+  end


To follow the http://www.betterspecs.org/ convention, I would write these tests like this.
(Take it or leave it 😄)

describe '#recommended_for?' do let(:base) { create :product } let(:extension) { create(:product, :extension, base_products: [base], recommended: recommended) } subject { extension.recommended_for?(queried_base) } context 'when the extension is recommended for its base' do let(:recommended) { true } let(:queried_base) { base } it { is_expected.to be true } end context 'when the extension is not recommended for its base' do let(:recommended) { false } let(:queried_base) { base } it { is_expected.to be false } end context "when the queried base is not the extension's base" do let(:recommended) { true } let(:queried_base) { create(:product) } it { is_expected.to be false } end end

Yep, I definitely like it! Just commit those changes yourself, maybe?

lagartoflojo · 2018-01-23T16:08:12Z

🙈 rubocop....

ikapelyukhin · 2018-01-23T16:14:42Z

🤖

ikapelyukhin added the WIP Work in progress, do not merge. label Jan 3, 2018

Modify the schema and the existing specs

84dd088

ikapelyukhin force-pushed the sles15_product_tree_support branch from 1f30ffb to 84dd088 Compare January 3, 2018 14:21

ikapelyukhin and others added 5 commits January 4, 2018 16:48

Modify serializer and SCC sync to walk products recursively

f022bcd

Save correct recommended value, add available attribute

c5eed3a

Add specs for SCC sync with SLE15 product tree

df59f3a

Merge branch 'master' into sles15_product_tree_support

07520c7

Fix fixture filename and rubocop offenses

61663f7

ikapelyukhin force-pushed the sles15_product_tree_support branch from 07a3cab to 61663f7 Compare January 17, 2018 12:39

ikapelyukhin removed the WIP Work in progress, do not merge. label Jan 17, 2018

ikapelyukhin changed the title ~~SLE15 product tree support~~ SLE15 product tree support -- 4702 Jan 17, 2018

suse-tests-pass assigned lagartoflojo Jan 17, 2018

Add specs for ProductSerializer

d1ffc94

lagartoflojo requested changes Jan 18, 2018

View reviewed changes

ikapelyukhin added 5 commits January 22, 2018 13:23

Skip repositories for extensions with unavailable base products

80e6d50

Add specs for extensions without base products

b6543e7

Merge branch 'master' into sles15_product_tree_support

57e9038

Fix the branch after merge

10bed41

Fix spec setup for Travis

74a495f

Fix token saving and add missing specs for that

1b9e50e

lagartoflojo requested changes Jan 23, 2018

View reviewed changes

Add specs for #recommended_for?

0499743

lagartoflojo approved these changes Jan 23, 2018

View reviewed changes

Refactor #recommended_for? specs

dcfa1aa

Fix Rubocop offenses

629adfd

ikapelyukhin merged commit 0ad46d8 into master Jan 23, 2018

ikapelyukhin deleted the sles15_product_tree_support branch January 23, 2018 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SLE15 product tree support -- 4702 #72

SLE15 product tree support -- 4702 #72

ikapelyukhin commented Jan 3, 2018

ikapelyukhin commented Jan 17, 2018

suse-tests-pass commented Jan 17, 2018

ikapelyukhin commented Jan 17, 2018

lagartoflojo left a comment

lagartoflojo Jan 18, 2018

ikapelyukhin Jan 22, 2018

lagartoflojo Jan 22, 2018 •

edited

Loading

ikapelyukhin Jan 22, 2018

ikapelyukhin Jan 22, 2018

lagartoflojo Jan 22, 2018

ikapelyukhin Jan 22, 2018 •

edited

Loading

lagartoflojo Jan 23, 2018

ikapelyukhin Jan 25, 2018

lagartoflojo Jan 18, 2018

ikapelyukhin Jan 22, 2018

lagartoflojo Jan 22, 2018

lagartoflojo commented Jan 18, 2018

lagartoflojo Jan 18, 2018

ikapelyukhin Jan 22, 2018

lagartoflojo Jan 22, 2018 •

edited

Loading

ikapelyukhin commented Jan 22, 2018

lagartoflojo left a comment

lagartoflojo Jan 23, 2018

ikapelyukhin Jan 23, 2018

lagartoflojo left a comment

lagartoflojo Jan 23, 2018

ikapelyukhin Jan 23, 2018

lagartoflojo Jan 23, 2018 •

edited

Loading

lagartoflojo commented Jan 23, 2018

ikapelyukhin commented Jan 23, 2018

SLE15 product tree support -- 4702 #72

SLE15 product tree support -- 4702 #72

Conversation

ikapelyukhin commented Jan 3, 2018

ikapelyukhin commented Jan 17, 2018

suse-tests-pass commented Jan 17, 2018

ikapelyukhin commented Jan 17, 2018

lagartoflojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lagartoflojo Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikapelyukhin Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lagartoflojo commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lagartoflojo Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

ikapelyukhin commented Jan 22, 2018

lagartoflojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lagartoflojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lagartoflojo Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

lagartoflojo commented Jan 23, 2018

ikapelyukhin commented Jan 23, 2018

lagartoflojo Jan 22, 2018 •

edited

Loading

ikapelyukhin Jan 22, 2018 •

edited

Loading

lagartoflojo Jan 22, 2018 •

edited

Loading

lagartoflojo Jan 23, 2018 •

edited

Loading