Skip to content

Change not to create history when destroying with force_update#135

Merged
wata727 merged 1 commit intokufu:masterfrom
wata727:force_destroy
Jul 18, 2023
Merged

Change not to create history when destroying with force_update#135
wata727 merged 1 commit intokufu:masterfrom
wata727:force_destroy

Conversation

@wata727
Copy link
Copy Markdown
Contributor

@wata727 wata727 commented Jun 27, 2023

This PR changes the destroy behavior on force_update. Previously, when destroying, the state before destruction was added as history.

employee = Employee.create!
employee.destroy!

puts ActiveRecord::Bitemporal::Visualizer.visualize(employee)

# transaction_datetime    | valid_datetime
#                         | 2023-06-27 16:23:24.487
#                         |                   | 2023-06-27 16:23:52.387
#                         |                   |                   | 9999-12-31 09:00:00.000
# 2023-06-27 16:23:24.487 +---------------------------------------+
#                         |                                       |
#                         |                                       |
#                         |                                       |
#                         |                                       |
# 2023-06-27 16:23:52.387 +-------------------+-------------------+
#                         |*******************|
#                         |*******************|
#                         |*******************|
#                         |*******************|
# 9999-12-31 09:00:00.000 +-------------------+

After this change, destruction performed within force_update will no longer create history.

employee = Employee.create!
employee.force_update { employee.destroy! }

puts ActiveRecord::Bitemporal::Visualizer.visualize(employee)

# transaction_datetime    | valid_datetime
#                         | 2023-06-27 08:42:13.857
#                         |                                       | 9999-12-31 00:00:00.000
# 2023-06-27 08:42:13.857 +---------------------------------------+
#                         |***************************************|
#                         |***************************************|
#                         |***************************************|
#                         |***************************************|
#                         |***************************************|
#                         |***************************************|
#                         |***************************************|
#                         |***************************************|
#                         |***************************************|
# 2023-06-27 08:42:26.857 +---------------------------------------+

This is an application of force_update's "operate without saving history" property to destruction.

This is useful for destruction without history if you accidentally creates a record. This is equivalent to update_columns(transaction_to: Time.current), but is implemented as an Active Record interface to benefit from standard mechanisms such as callbacks.

@wata727 wata727 requested review from Dooor and krororo July 18, 2023 05:15
it { expect { subject }.not_to change(employee, :valid_to) }
it { expect { subject }.not_to change(employee, :transaction_from) }
it { expect { subject }.to change(employee, :transaction_to).from(ActiveRecord::Bitemporal::DEFAULT_TRANSACTION_TO).to(destroyed_time) }
it { expect { subject }.not_to change { Employee.ignore_valid_datetime.within_deleted.count } }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

review:nits
It looks like ignore_bitemporal_datetime can be used.

Suggested change
it { expect { subject }.not_to change { Employee.ignore_valid_datetime.within_deleted.count } }
it { expect { subject }.not_to change { Employee.ignore_bitemporal_datetime.count } }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. But ignore_valid_datetime.within_deleted is the standard way in the spec.
https://github.com/search?q=repo%3Akufu%2Factiverecord-bitemporal+ignore_valid_datetime.within_deleted&type=code

It would be nice to replace them all in someday.

Copy link
Copy Markdown
Collaborator

@krororo krororo left a comment

Choose a reason for hiding this comment

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

I made a nits comment, but it looks good.

Copy link
Copy Markdown
Contributor

@Dooor Dooor left a comment

Choose a reason for hiding this comment

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

👍

@wata727 wata727 merged commit ccd2d80 into kufu:master Jul 18, 2023
@wata727 wata727 deleted the force_destroy branch July 18, 2023 10:24
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