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

Wheels save(allowExplicitTimestamps=true) doesn't produce the expected result #1113

Merged
merged 4 commits into from May 3, 2022

Conversation

SebastienFCT
Copy link
Contributor

@SebastienFCT SebastienFCT commented Mar 27, 2022

Hello there,

I found some confusion in the documentation regarding the usage of allowExplicitTimestamps. After looking into wheels source code, it seems allowExplicitTimestamps has been wrongly indicated as a parameter for the model.save() function while it's missing for model.new() (note: model.create() is properly documented).

I've updated their signatures:

  • I've removed allowExplicitTimestamps=true from model.save() signature as it doesn't produce the effect indicated in the documentation. The following test would show the error:
component extends="wheels.tests.Test" {

	function test_override_created_at_with_allow_explicit_timestamps() {

		author = model("author").findOne(order = "id");
		
		transaction {

			twentyDaysAgo = dateAdd("d", -20, now());
			
			newPost = model("post").new();
			newPost.authorId = author.id;
			newPost.title = "New title";
			newPost.body = "New Body";
			newPost.createdAt = twentyDaysAgo;
			newPost.updatedAt = twentyDaysAgo;

			newPost.save(transaction="none", allowExplicitTimestamps=true);  // Note: `allowExplicitTimestamps` has no effect here and should be added to the `new` call instead
			
			assert("newPost.createdAt == twentyDaysAgo");
			
			transaction action="rollback";
		}
	}
}
  • I've added allowExplicitTimestamps = false to the model.new() signature. While this isn't required (it's passed through arguments.properties) I thought it'd be more consistent with the existing model.create() signature.
  • I've added a couple tests for both create and new to show how the allowExplicitTimestamps should be used
  • I've updated the test model.properties.properties.test_nested_property_is_not_returned_when_returnincluded_is_false to expect the new explicit property from new()
  • I've updated the test model.properties.properties.test_setting_and_getting_properties_with_named_arguments to expect the new explicit property from new()
  • I've updated the test model.properties.properties.test_setting_and_getting_properties to expect the new explicit property from new()
  • I've removed the test model.properties.timestamps.test_allowexplicittimestamps_is_not_an_object_property as it's not relevant anymore now that the property is defaulted.

The documentation should also be updated. Specifically the following pages (I'm not sure how to get this done):

  • model.save: allowExplicitTimestamps should be removed from the list of parameters
  • model.new: allowExplicitTimestamps should be added to the list of parameters

@bpamiri
Copy link
Collaborator

bpamiri commented May 3, 2022

@SebastienFCT Thank you for the contribution.

@bpamiri bpamiri merged commit 7eb87bf into cfwheels:master May 3, 2022
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.

None yet

2 participants