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

Add skip_timestamps option #486

Conversation

stufro
Copy link
Contributor

@stufro stufro commented Sep 27, 2023

Resolves #318

This adds the ability to pass skip_timestamps: true to update and update! methods. To do this I had to change the save method to accept skip_timestamps so to avoid inconsistent behaviour I have also added the option to the create and create! methods.

To achieve this I had to remove the type definition of args on the method definition so that either a Granite::ModelArgs or a NamedTuple can be passed in with skip_timestamps, I originally added another method overload for this case as you can see in the first commit e6f917e. Open to thoughts on this.

I have also updated the crud.md docs to describe this feature and added a missing example for save(validate: false)

@stufro stufro force-pushed the 318-update-without-changing-timestamps branch from 5a545eb to 1ad352b Compare September 27, 2023 18:08
@robacarp
Copy link
Member

Huh. I have wanted #update with skip_timestamps but never #create. Typically I want that because I want to "soft touch" some record without altering the breadcrumb trail for some reason...which I can't think of why I'd want to do that on a create.

I think the timestamp columns are specified as NOT NULL (or at least they are in my databases). Wouldn't that cause a problem?

@stufro stufro force-pushed the 318-update-without-changing-timestamps branch from 35e9eca to 177c930 Compare October 1, 2023 11:37
@stufro
Copy link
Contributor Author

stufro commented Oct 1, 2023

Hmm yeah you're right. I hadn't considered DB level constraints.

I've removed it from create and also squashed some of the commits to tidy up the branch.

@crimson-knight
Copy link
Member

@stufro I think that you should keep the option to create records while skipping timestamps if the created_at and updated_at values are already present. That's the current gap, creating records in bulk that already have timestamps that want to be preserved but they aren't at the time of creation.

Database constraints will error on their own for the user, so I wouldn't worry about policing that from inside of Granite. I've had instances where we did imports and ignored the existence of the timestamps during creation but allowed them to be updated if/when the record was updated.

@stufro
Copy link
Contributor Author

stufro commented Oct 16, 2023

@crimson-knight ah yes I suppose that might also help with resolving #481?

I have made the change.

@stufro stufro force-pushed the 318-update-without-changing-timestamps branch from 1f8911b to 495776c Compare October 16, 2023 10:20
@stufro stufro force-pushed the 318-update-without-changing-timestamps branch from 495776c to c467b4a Compare October 16, 2023 10:22
@crimson-knight crimson-knight merged commit 33f2dfb into amberframework:master Oct 16, 2023
5 of 13 checks passed
@stufro stufro deleted the 318-update-without-changing-timestamps branch October 28, 2023 08:58
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.

.update() without changing timestamps
3 participants