Do not commit to db unchanged products in bulk save

[isidzukuri]

What? Why?

• Closes [BUU] Avoid validating all records when saving #12503

It imprves performance.

What should we test?

• enable feature toggle admin_style_v3
• Set up a product with invalid tax category (see Show any tax category errors when saving variants #12486 (comment)). This will make the product invalid.
• visit /admin/products
• saving of other products should work
  • unchanged invalid product should be ignored, with no error message.
  • saving time may be improved.
  • (in DEV: in logs should be visible db commit transactions only for updated products )

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

Dependencies

Documentation updates

[dacook]

I hope you don't mind me updating this, because I think it is ready for review now :D

[isidzukuri]

cool! Thank you! 👍

[rioug]

Nice one, but I think we are missing testing on one of the changes David made (see comment ), could you update the specs ? thanks !

[rioug]

It's a bit of a nit pick, but using subject here without knowing the context doesn't tell us what's happening. It'd rather be explicit and use product_set.save so I know straight away what we are testing.
Same for the various use of subject below.

[isidzukuri]

I haven't seen anywhere in the project style guide about it. So its comes down to personal preferences. In such cases i follow the betterspecs.org. If talking particularly about subject a lot arguments are here betterspecs/betterspecs#7

I understand that exist different styles of writing tests. Another one is to put all context definition into into each it. Its described in book "Testing Rails"(Josh Steiner, Joël Quenneville) https://books.thoughtbot.com/assets/testing-rails.pdf . But it does not looks like OFN follows this approach either.

but using subject here without knowing the context doesn't tell us what's happening

Sounds like a code smell. Why? Because subject describes what tested subject actually does. And its good when it does only one thing, because of Single Responsibility principle (SOLID). From my experience: If its not possible to write simple, obvious unit tests using rspec interface - code design is not optimal. Is the class ProductSet really that bad? Well, it uses inheritance, method override. include, extend, metaprogramming.... and it makes it sophistiated. But still its possible to make assumption that main(!) responsibility is to persist some data, and it is described here:

openfoodnetwork/spec/services/sets/product_set_spec.rb

Lines 5 to 10 in 90c71c6

	RSpec.describe Sets::ProductSet do
	describe '#save' do
	let(:product_set) do
	described_class.new(collection_attributes: collection_hash)
	end
	subject{ product_set.save }

It follows rspec convention. So its good enough for me at the moment as for legacy code.

For context definition exists keyword context. Maybe it worth to extend description at this line https://github.com/openfoodfoundation/openfoodnetwork/pull/12515/files#diff-385c5ce991f97a247a8510a960775290559a04d5f0f4007219bdd346db00cc8eR138 to make it more obvious?

[isidzukuri]

or the spec can also look like:

expect {subject}.not_to change { product_set.saved_count }.from( 0 )

[rioug]

Sorry I should have expressed myself better, it's more of a "personal nit pick". I am aware of the better spec recommendations, I just don't agree with all of it. But as you say it comes down to personal preference.
Anyway I am happy for the code to be merged like this, the requested change was in relation to David's changes, which he updated. So all good now !

[rioug]

Thanks for your help @isidzukuri ! The PR looks good now.

[rioug]

Sorry I should have expressed myself better, it's more of a "personal nit pick". I am aware of the better spec recommendations, I just don't agree with all of it. But as you say it comes down to personal preference.
Anyway I am happy for the code to be merged like this, the requested change was in relation to David's changes, which he updated. So all good now !

[mkllnk]

Nice to see this collaboration. It's not easy to work on legacy code. Well done.

[filipefurtad0]

Hey @isidzukuri ,

Thanks for working on this. I've followed your steps, and looked at the console. I think I've spotted the DB transaction for the non-updated product as well:

In any case, the PR seems not to introduce any regression. I'm just not sure it is bringing the expected improvement. What do you think?

[isidzukuri]

hi @filipefurtad0 ! Thank you for checking!
This line of log is related to all parameters sent from UI to rails controller. Logs of queries to DB looks slightly different. For example:

Article Load (0.2ms)  SELECT "articles".* FROM "articles" /*application='Blog',controller='articles',action='index'*/
Article Update (0.3ms)  UPDATE "articles" SET "title" = ?, "updated_at" = ? WHERE "posts"."id" = ? /*application='Blog',job='ImproveTitleJob'*/  [["title", "Improved Rails debugging guide"], ["updated_at", "2022-10-16 20:25:40.091371"], ["id", 1]]

It should include SQL statements like UPDATE, CREATE, SELECT...
I don't see any at this screenshot. So it looks as expected to me.

Additional info is here:

• https://guides.rubyonrails.org/debugging_rails_applications.html#the-logger
• https://guides.rubyonrails.org/debugging_rails_applications.html#sql-query-comments

[filipefurtad0]

Hey @isidzukuri :-)

Thank you so much for your your help!

(in DEV: in logs should be visible db commit transactions only for updated products )

Shouldn't we be seeing that UPDATE line on updated product?

I was thinking one reason for not seeing it, could be that we'd have to enable this option (according to the documentation you've sent):

config.active_record.query_log_tags_enabled = true

I've tried adding it to /config/application.rb and restarting both the server and the puma service, but still no luck.

What do you think? Thanks again for your help 🙏

[isidzukuri]

@filipefurtad0 On valid updated product - yes, log should be visible. I saw it locally, in development environment, with default settings. It looks like:

[isidzukuri]

in other environments this log may be disabled. If so, to check if product was updated i can see two options: enable logging or check updated_at column.

[filipefurtad0]

Yes, indeed this type of log seems to be disabled. I was not able to set it up, so I was not able to check this type of commits. However...

check updated_at column

This is indeed a great idea! Thank you 👍

So we can see that, despite the invalid new variant, the existing variant is edited (updated_at changes), but no new variant is created (variant count stays the same).

Alright, merging!

[isidzukuri]

@filipefurtad0 great 🚀

PS: may be useful for the future, log should be enabled by adding ActiveRecord::Base.logger.level = :debug somewhere in the code before query execution, for example to /config/application.rb as you wrote previously