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

Soft deletes use deleted_at #2051

Merged
merged 19 commits into from
Jun 12, 2019
Merged

Soft deletes use deleted_at #2051

merged 19 commits into from
Jun 12, 2019

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Jun 10, 2019

Description
First attempt. Change all references to deleted to use deleted_at instead. Direct references to these fields now use dates, and tests check IS NULL and IS NOT NULL instead of binary values.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

MGatner added 13 commits June 10, 2019 10:25
@MGatner
Stash insert ID before event trigger
72ab824 
If any afterInsert events call their own database insert then `db->insertID()` will return that key instead of the desired version. This change grabs the insert ID before triggering the event so it can be returned (if requested) after.
@MGatner
Update to use deleted_at instead of deleted
44a0235 
* Change field used from deleted to deleted_at
* Change binaries test to null / is not null
@MGatner
Adjust tests for deleted_at
c46be1c 
* Seed values with date instead of 1
* Test IS NULL / IS NOT NULL instead of binaries
@MGatner
Change allowedFields for deleted_at
f31d777
@MGatner
Change guide to reflect deleted_at
5a23efc
@MGatner
Use new deleted_at field
d67b488
@MGatner
Change deleted to deleted_at fields
9ea915b
@MGatner
Change expected XML return for new deleted_at
7dbc8fa
@MGatner
Implement deleted_at
8617776
@MGatner
Change deleted example
f4c3761
@MGatner
Fix CSV assertion for null deleted_at
8abbef6
@MGatner
Fix order of XML assertion
48764f2
@MGatner
Change deleted to deleted_at
6a41a09
@MGatner
Copy link
Member Author

MGatner commented Jun 10, 2019 •
edited
Loading

@lonnieezell I might need some help. I think everything is correct now but I replaced 'deleted' => 0 with 'deleted_at IS NULL' (and their opposites) and the PostGres handler doesn't seem to be interpreting the string as a query fragment (https://travis-ci.org/codeigniter4/CodeIgniter4/jobs/543886513). I think it is an issue in seeInDatabase() and not the framework's whereHaving() but I'm not familiar with why PostGres is taking this as an int (or maybe expecting an int?). The users table is definitely defining date fields as datetimes, just mock events has the int versions of them so I don't think it is related.

@MGatner
Copy link
Member Author

MGatner commented Jun 10, 2019 •
edited
Loading

Hmm in retrospect I wonder if it would accept 'deleted_at' => null as a valid criterium and translate it? I'll wait for your response but that might be something to test next.

@MGatner MGatner mentioned this pull request Jun 11, 2019
Closed
lonnieezell
lonnieezell requested changes Jun 11, 2019
View reviewed changes
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Pretty close! Thanks so much, I ran out of time to look into it last night.

system/Model.php Show resolved Hide resolved
system/Model.php Outdated Show resolved Hide resolved
user_guide_src/source/models/model.rst Outdated Show resolved Hide resolved
@MGatner
Copy link
Member Author

MGatner commented Jun 12, 2019

IMO this merge would merit a blog entry of some kind, similar to the entity refactor, as it will require developers to intervene with database changes upon upgrading.

lonnieezell
lonnieezell requested changes Jun 12, 2019
View reviewed changes
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Looks like I missed 3 spots that need table prefixes in previous review.

system/Model.php Outdated Show resolved Hide resolved
system/Model.php Outdated Show resolved Hide resolved
system/Model.php Outdated Show resolved Hide resolved
@lonnieezell
Copy link
Member

Well that was quick! :) Looks good. Thanks for all of this!

@lonnieezell lonnieezell merged commit 0d16d45 into codeigniter4:deletedat Jun 12, 2019
@MGatner
Copy link
Member Author

MGatner commented Jun 12, 2019

I can't tell you how glad I will be to remove all my explicit code for deleted_at fields on everything.

@MGatner MGatner deleted the deletedat branch June 12, 2019 14:03
@lonnieezell
Copy link
Member

I can't tell you how glad I will be to remove all my explicit code for deleted_at fields on everything.

Well - glad I could help create more work for you and have you be happy about it :)

@MGatner
Copy link
Member Author

MGatner commented Jun 12, 2019

Haha well more work now for less work later, my kinda work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lonnieezell lonnieezell lonnieezell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MGatner @lonnieezell