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

Fix Rubocop Rails: Rails/HasManyOrHasOneDependent #12313

Merged
merged 8 commits into from
Apr 25, 2024
Merged

Fix Rubocop Rails: Rails/HasManyOrHasOneDependent #12313

merged 8 commits into from
Apr 25, 2024

Conversation

anthonyms
Copy link
Contributor

What? Why?

Fix all violations of the Rails/HasManyOrHasOneDependent cop.

Rubocop requires has_many or has_one associations to specify a :dependent option

What should we test?

N/A

Release notes

  • Technical changes only

@anthonyms anthonyms force-pushed the 11482-fix-rubocop-rails-issue-has_many branch from 1e0b382 to 27ced44 Compare March 26, 2024 16:17
@rioug rioug added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Mar 26, 2024
rioug
rioug reviewed Mar 26, 2024
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Thanks for your help ! I have some reservation on some of the changes that have been made.
We need to think a bit more about what we delete or not when deleting an enterprise. Maybe we should also use act_as_paranoid on Enterprise ? Or maybe we need to replace the enterprise by a place holder, ie : https://world.hey.com/jorge/code-i-like-i-domain-driven-boldness-71456476 ?
ping @openfoodfoundation/developers

app/models/enterprise.rb Outdated Show resolved Hide resolved
app/models/spree/stock_item.rb Outdated
@@ -8,7 +8,7 @@ class StockItem < ApplicationRecord

belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items
belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant'
has_many :stock_movements
has_many :stock_movements, dependent: nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure why we would want to keep stock_movements on a delete stock item, but maybe I am missing some context.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We should destroy stock movements.

Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to destroy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

app/models/enterprise.rb Outdated Show resolved Hide resolved
mkllnk
mkllnk requested changes Mar 26, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. It's impossible for you to know how our business logic works. So we added some comments for the behaviour we would like for now.

app/models/enterprise.rb Outdated Show resolved Hide resolved
app/models/enterprise.rb Outdated Show resolved Hide resolved
app/models/spree/stock_item.rb Outdated
@@ -8,7 +8,7 @@ class StockItem < ApplicationRecord

belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items
belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant'
has_many :stock_movements
has_many :stock_movements, dependent: nil
Copy link
Member

Choose a reason for hiding this comment

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

I agree. We should destroy stock movements.

app/models/spree/tax_rate.rb Show resolved Hide resolved
app/models/enterprise.rb Outdated Show resolved Hide resolved
@anthonyms anthonyms force-pushed the 11482-fix-rubocop-rails-issue-has_many branch 2 times, most recently from 67c2df3 to 465bf40 Compare April 3, 2024 11:43
mkllnk
mkllnk requested changes Apr 4, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Much better, I just found one last thing to change.

app/models/spree/stock_item.rb Outdated
@@ -8,7 +8,7 @@ class StockItem < ApplicationRecord

belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items
belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant'
has_many :stock_movements
has_many :stock_movements, dependent: nil
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to destroy?

@anthonyms anthonyms force-pushed the 11482-fix-rubocop-rails-issue-has_many branch from 465bf40 to fc4b59d Compare April 9, 2024 09:23
@anthonyms
Copy link
Contributor Author

Thank you for working on this. It's impossible for you to know how our business logic works. So we added some comments for the behaviour we would like for now.

Thanks @mkllnk
Regarding the stock_item -> stock_movement relationship, Do you have context as to why StockMovement is marked as read_only (and not acts_as_paranoid)? The read_only attribute prevents it from being deleted

@mkllnk
Copy link
Member

mkllnk commented Apr 10, 2024

Do you have context as to why StockMovement is marked as read_only

That model came from the Spree framework and it seems like they didn't account for deleting any data. There are not callbacks registered on the model, so could just do a delete.

@anthonyms
Fix Rubocop Rails issue: Rails/HasManyOrHasOneDependent
5559816
@anthonyms
Fix Rubocop issue: Do not delete addresses having shipments
1ec453d 
The reasoning is that we should not delete an address that has
ever received a shipment
@anthonyms
Fix Rubocop: Do not delete dependent stock_movements
4f851bb
@anthonyms
Fix Rubocop: Do not delete dependent adjustments
4140257 
TaxRate acts_as_paranoid iand is thus not hard_deleted
@anthonyms
Fix Rubocop: Do not delete Spree::Variant associations
645cb10 
Spree::Variant acts_as_paranoid and is thus not hard deleted
@anthonyms
Fix Rubocop: Hard delete paranoid associations
c2cbe4f 
As much as the associated models act_as_paranoid, it
doesnt make sense to keep them around after deleting the enterprise
@anthonyms
Fix Rubocop: Update handling of enterprise associations
434afb7
@anthonyms anthonyms force-pushed the 11482-fix-rubocop-rails-issue-has_many branch 2 times, most recently from f70ea27 to 293ce91 Compare April 23, 2024 10:00
@anthonyms anthonyms requested review from rioug and mkllnk April 23, 2024 10:08
@anthonyms
Copy link
Contributor Author

@rioug @mkllnk
Re-requested review

@anthonyms anthonyms force-pushed the 11482-fix-rubocop-rails-issue-has_many branch from 293ce91 to 0d03cdf Compare April 23, 2024 10:13
rioug
rioug approved these changes Apr 23, 2024
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks good now 👍 Thanks for your help !

mkllnk
mkllnk approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Thank you!

@mkllnk
Copy link
Member

mkllnk commented Apr 23, 2024

Testing:

  • Deleting products.

@filipefurtad0 filipefurtad0 self-assigned this Apr 25, 2024
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Apr 25, 2024
@filipefurtad0
Copy link
Contributor

Hey @mkllnk ,

Deleting products.

Thanks for pointing that out. I've staged the PR and verified that deleting both products and variants work as before:

  • the product disappears; refreshing the page does not display the deleted product
  • when previously added the product is removed from order cycles
  • deleting products does not change previously placed orders (i.e., line item quantities are not changed, if a product is deleted)

Looks good, merging.

@filipefurtad0 filipefurtad0 merged commit e99b072 into openfoodfoundation:master Apr 25, 2024
52 checks passed
@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@rioug rioug rioug approved these changes

Assignees

@anthonyms anthonyms

@filipefurtad0 filipefurtad0

Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rubocop Rails/HasManyOrHasOneDependent
4 participants
@anthonyms @mkllnk @filipefurtad0 @rioug