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

added a test case to cover the bug #531

Merged

Conversation

sherif-nedap
Copy link
Contributor

No description provided.

@sherif-nedap
Copy link
Contributor Author

Issue is occurring because of this way of determining the deletion_time is not taking into account custom columns with datatype different than datetime
in this case a Boolean :active

so here it will try to compare the recovery_window_range to the active column
and it should instead compare it to the deleted_at

this used to work fine before because deleted_at were hard coded

  def within_recovery_window?(recovery_window_range)
    return true unless recovery_window_range
    recovery_window_range.cover?(deletion_time)
  end

@mathieujobin
Copy link
Collaborator

@sherif-nedap @sas1ni69

I ran this in a debugger, I don't quite understand, why is the value of active being returned as deletion_time ?

That seems wrong...
I can make the test pass by making this change... but I am not sure it makes sense

image

@sas1ni69
Copy link
Contributor

@mathieujobin yeah that doesn't seem right. I've separated both values from each other. I have a PR open here https://github.com/rubysherpas/paranoia/pull/532/files

Let me know if this works for you 🙏🏻

@sas1ni69
Copy link
Contributor

Also, how would I include @sherif-nedap 's test as his own commit?

Really appreciate the test case

@mathieujobin mathieujobin merged commit e7c5a34 into rubysherpas:core Mar 23, 2022
karunkumar1ly pushed a commit to edcast/paranoia that referenced this pull request Feb 6, 2024
Co-authored-by: Sherif Elkassaby <sherif.elkassaby@nac37075.nedap.local>
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.

3 participants