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.

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

Fixed Inconsistency in column_type, allow_nulls, recovery_value #193

Merged

Conversation

soartec-lab
Copy link
Contributor

Thank you for providing such a great gem.
I created this PR because I ran into one problem while using this gem in my project.

Expect

In case of column type:" boolean " and allow nulls: false, nil should not be stored in the deleted column.

Actual

However, when the recover and recover! methods are used for the object of the class with the following conditions, nil is stored in the deleted column.

class ParanoidBooleanNotNullable < ActiveRecord::Base
  acts_as_paranoid column: "deleted", column_type: "boolean", allow_nulls: false
end

ParanoidBooleanNotNullable.create!
ParanoidBooleanNotNullable.first.destroy
ParanoidBooleanNotNullable.delete_flag # => true
ParanoidBooleanNotNullable.only_deleted.first.recover # => true
ParanoidBooleanNotNullable.delete_flag # => nil 'expected is false'

Changes

Set default value recovery_value to false for case ofcolumn_type:" boolean " and allow_nulls: false.

Copy link
Contributor

@mvz mvz left a comment

Choose a reason for hiding this comment

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

This makes perfect sense. Can you take a look at the test failures?

@@ -5,6 +5,7 @@
require "acts_as_paranoid/associations"
require "acts_as_paranoid/validations"
require "acts_as_paranoid/relation"
require "pry-byebug"
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests are failing because of this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this and committed again.

@soartec-lab soartec-lab force-pushed the fix/inconsistency-in-paranoid-boolean branch from 63bbca2 to 4ae6596 Compare December 19, 2020 07:48
@soartec-lab
Copy link
Contributor Author

@mvz

Thank you for your review. It seems that I forgot to delete the library used during development.
I removed this and confirmed that the test was all green.
could you review again?

@soartec-lab soartec-lab requested a review from mvz December 19, 2020 07:54
@soartec-lab
Copy link
Contributor Author

I'm experiencing some Rubocop Offenses. However, to address this warning, a refactoring that is not significantly involved in this fix is ​​required. I'm thinking of proposing this fix in another PR again after merging this PR.
Is this PR mergeable?

@mvz
Copy link
Contributor

mvz commented Dec 19, 2020

Please don't worry about the RuboCop offenses for now.

@soartec-lab
Copy link
Contributor Author

Was good. I was relieved.

@soartec-lab
Copy link
Contributor Author

soartec-lab commented Dec 20, 2020

@mvz
The tests are all green, is this PR ready to merge?
Or do I have anything else to do?

@zhengf2
Copy link

zhengf2 commented Apr 15, 2021

Hi @mvz , is there something holding up this PR that needs addressing? We've hit this issue when trying to upgrade to 0.7.2 as well

@mvz
Copy link
Contributor

mvz commented Apr 15, 2021

Thanks for reminding me @zhengf2. I'll check this again this weekend.

@mvz mvz self-assigned this Apr 15, 2021
@mvz mvz merged commit 5491972 into ActsAsParanoid:master Apr 16, 2021
@mvz
Copy link
Contributor

mvz commented Apr 16, 2021

Thanks, @soartec-lab!

@mvz
Copy link
Contributor

mvz commented Apr 16, 2021

This change has been released as part of version 0.7.2.

@soartec-lab soartec-lab deleted the fix/inconsistency-in-paranoid-boolean branch April 16, 2021 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants