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

Archivable behavior with FK customization. #1484

Closed
wants to merge 2 commits into from
Closed

Archivable behavior with FK customization. #1484

wants to merge 2 commits into from

Conversation

ppatrik
Copy link

@ppatrik ppatrik commented Jun 23, 2018

Added two boolan parameters:

  • archive_foreign_keys (default: false) - on true FK will be copied to Archive table class, so API for archive and non-archive class is the same.
  • archive_foreign_keys_skip_sql (default: true) - same API is enought so database FK will be skiped by default.

ppatrik added 2 commits June 23, 2018 15:34
@ppatrik
Archivable behaviour with FK customization.
3abce00 
Added two boolan parameters:
* archive_foreign_keys (default: false) - on true FK will be copied to Archive table class, so API for archive and non-archive class is the same.
* archive_foreign_keys_skip_sql (default: true) - same API is enought so database FK will be skiped by default.
@ppatrik
Fixed parameter checks.
b14aca6
Incognito
Incognito suggested changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@Incognito Incognito left a comment

Choose a reason for hiding this comment

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

I like this and it works with the latest build when rebased on master.

I would like to see a test that proves it works as intended and will ensure nobody breaks it in the future.

@dereuromark dereuromark changed the title Archivable behaviour with FK customization. Archivable behavior with FK customization. Dec 19, 2022
@dereuromark
Copy link
Contributor

@ppatrik or others: Any update on this?

@Incognito
Copy link
Contributor

@dereuromark It's a useful improvement but I don't think ppatrik is coming back to add a test after 4 years. Also, I'm not sure if anyone is actively using Archive functionality in the wild or not. If you think end-users aren't using it: maybe close it with a flag "icebox" or something to indicate it can be pulled back if there is a need for it.

@dereuromark dereuromark added the Iceboxed Closed, but can be reopened or redone in follow-up PR label Dec 19, 2022
@mringler
Copy link
Contributor

@Incognito I think Archive is used, at least there was a question on the issue tracker from someone using it: #1919#issuecomment-1340073429
I think the problem even was about missing FKs

@Incognito
Copy link
Contributor

Sounds relevant IMO, that issue could probably be fixed with this MR. I'm no longer a maintainer but I know at the time the code LGTM in concept, but it was missing a test to ensure it doesn't break in the future.

Migrating from old sf components to modern ones was a massive pain and comprehensive tests was the only way it was possible without breaking things.

@mringler mringler mentioned this pull request Dec 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Incognito Incognito Incognito requested changes

Assignees
No one assigned
Labels
Iceboxed Closed, but can be reopened or redone in follow-up PR Improvement Tests Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ppatrik @dereuromark @Incognito @mringler