Updates Expires attribute on Model Updates #420

devotox · 2018-09-05T10:11:50Z

Summary:

Allows the Expires attribute to be updated everytime the model updates.
This makes sense because you would want something to expire after it has not been touched for a certain amount of time not just from the time it was created

Type (select 1):

Is this a breaking change? (select 1):

🚨 YES 🚨
No
I'm not sure

Is this ready to be merged into Dynamoose? (select 1):

Yes
No

Are all the tests currently passing on this PR? (select 1):

Yes
No

Other:

I have searched through the GitHub pull requests to ensure this PR has not already been submitted
I have updated the Dynamoose documentation (if required) given the changes I made
I have added/updated the Dynamoose test cases (if required) given the changes I made
I have run npm test from the root of the project directory to ensure all tests continue to pass
I agree that all changes made in this pull request may be distributed and are made available in accordance with the Dynamoose license
All of my commits and commit messages are detailed, explain what changes were made, and are easy to follow and understand
I have confirmed that all my code changes are indented properly using 2 spaces
I have filled out all fields above

coveralls · 2018-09-05T10:14:06Z

Pull Request Test Coverage Report for Build 663

17 of 19 (89.47%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.01%) to 84.566%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/Schema.js	2	4	50.0%

Files with Coverage Reduction	New Missed Lines	%
lib/Table.js	2	77.93%

Totals
Change from base Build 662:	-0.01%
Covered Lines:	1866
Relevant Lines:	2132

💛 - Coveralls

fishcharlie · 2018-09-05T20:27:53Z

@devotox Thanks for this! I think we do want to add documentation for this PR as well as write some tests for this.

I think we also need to confirm if this is a breaking change or not. We want to keep the existing behavior for the defaults as is I believe for this, and allow the user to pass in an option or something to overwrite the behavior.

devotox · 2018-09-10T10:05:05Z

Documentation updated & default is false so should not be a breaking change

fishcharlie

This is looking really good. Let's add some tests as well. I think we should add test(s) for both ensuring that the default behavior remains false, and behaves accordingly. As well as test(s) for ensuring that enabling that option and setting it to true behaves as expected. Any other ideas for tests are most welcome as well.

fishcharlie · 2018-09-11T01:08:04Z

lib/Model.js

@@ -539,6 +539,11 @@ Model.update = function(NewModel, key, update, options, next) {
    options.updateTimestamps = true;
  }

+  // default updateExpires to true


This should be false, correct? In this comment? Although it's just a comment, probably a good idea to ensure it is correct.

fishcharlie · 2018-09-11T01:11:43Z

@devotox Great work on this!! I think this will be a very welcome feature. Just left a review with a few comments.

Have you confirmed that this feature is not already built into Dynamoose? I'm pretty confident it has not been built into Dynamoose yet, but I just want to verify.

fishcharlie · 2018-09-11T01:12:09Z

@devotox This will be a very welcome feature and I know personally I will use it very extensively. So thank you for this!!

devotox · 2018-09-17T11:40:29Z

@fishcharlie yes this has not been built into Dynamoose yet. Looking to get some time to write the tests. tried to quickly throw some together based on updateTimestamps but alas it failed lol.

So gotta sit down and see why but comment updated at least

devotox · 2018-09-21T18:12:45Z

@fishcharlie Ive added tests but i noticed you are not testing updateTimestamps options in the schema tests. Shouldn't that be done and then also updateExpires.

At the moment you only test this on Model.save

devotox · 2018-09-21T18:22:13Z

I take that back you can't update timestamps from the schema 😝.

Are we sure we do not want updateExpires: true as default. it seems like it is something that should be.

fishcharlie · 2018-09-21T18:32:45Z

@devotox If you have a strong argument for changing the default/current behavior feel free to make that argument. I'm all for changing default behaviors if it is justified. It also has to take into account the fact that upgrading Dynamoose then will cause problems for people who are relying on the current behaviors.

If we draw the conclusion that we should change the current behavior this will need to be part of v1.0 probably. I will try to hold off on pushing v1.0 until we determine that, but I'm in the very final stages of that deployment, and a lot of my projects are gonna rely on v1.0, so we do kinda need to figure this out. If that doesn't make it into v1.0 we can always bring it up again for v2.0.

fishcharlie

Looking good. Let's add a few more tests. I think we should have a test to ensure the default is correct if we don't pass options into the .save function. And does this support the Model.update function. If so let's add tests for that. If not, it should probably be supported there as well.

fishcharlie · 2018-09-21T18:34:26Z

lib/Model.js

@@ -298,11 +298,19 @@ Model.prototype.put = function(options, next) {
      options.overwrite = true;
    }

-    let toDynamoOptions = {updateTimestamps: true};
+    let toDynamoOptions = {
+      updateExpires: false,


We don't need this, correct? It should just default to false by default without having this line.

I understand your point but isn't it better to have the default set so it is visible through the system that it is actually set to something?

@devotox I'm pretty sure the only reason to have it there is to increase readability and ease of understanding for developers in the future. Anytime we are comparing that value to false or true, undefined will behave just like false. So I don't think removing this is a bad thing. Especially since it looks like you did and all the tests are passing. If you believe there is an edge case that your tests haven't accounted for that this would break please write a test and if that test fails then yes we should totally add it back in.

fishcharlie · 2018-09-21T18:35:17Z

lib/Model.js

@@ -539,6 +547,11 @@ Model.update = function(NewModel, key, update, options, next) {
    options.updateTimestamps = true;
  }

+  // default updateExpires to false
+  if (typeof options.updateExpires === 'undefined') {
+    options.updateExpires = false;


Once again I don't think we need this section. 'undefined' is considered falsey so I don't think we need this part since the default will be undefined or false.

fishcharlie · 2018-09-21T18:38:13Z

test/Model.js

+
+        it('Save existing item without updating expires', function (done) {
+          var myCat = new Cats.Cat10({
+            id: 1,


Let's make sure this id is unique to ensure we aren't getting conflicts with the test above.

fishcharlie · 2018-09-21T18:39:05Z

test/Model.js

+              myCat.save({updateExpires: false}, function () {
+                Cats.Cat10.get(1, function(err, realCat) {
+                  realCat.name.should.eql("FluffyB");
+                  realCat.expires.should.eql(expectedExpires); // expires should be different than before


Shouldn't the comment read expires should be the same as before? Or something along those lines?

fishcharlie

LGTM

fishcharlie · 2018-09-24T16:20:10Z

@devotox This looks really good. Do you think we are ready to get this merged in or is there still more work to do?

devotox · 2018-09-25T08:52:52Z

@fishcharlie I just need to write the update tests. Yesterday when I tried couldn't get access to the updated expires attribute to compare them but will make another attempt this morning.

fishcharlie · 2018-09-25T14:14:26Z

@devotox Let me know if I can help at all!

fishcharlie · 2018-09-25T20:57:15Z

@devotox Looks like there are some merge conflicts as well. This is the last PR I'd like to get in before version 1.0. But if we can't get it done before version 1.0 we can hold off till version 1.1. I'm aiming on getting version 1.0 out hopefully this week or this weekend.

devotox · 2018-09-27T14:45:40Z

Haaa looks like the merge broke something .... gonna look into this

fishcharlie · 2018-09-27T15:02:36Z

@devotox Sounds good! And you still have one more test to write, correct?

devotox · 2018-09-30T01:17:23Z

@fishcharlie Test added. Probably too late for the Release but here it is

devotox · 2018-09-30T01:18:56Z

Also if this is a full version release we could technically make updateExpires default to true?

fishcharlie · 2018-09-30T01:25:07Z

@devotox Awesome. So is this ready to be merged in? Why do you believe defaulting to true is the better option here?

devotox · 2018-09-30T10:29:47Z

@fishcharlie Yes, i believe it can be merged. And I keep trying to think of a good argument or find something as proof but maybe I'm wrong. It just feels like that makes sense. "A document is expired a certain period from its last update"

fishcharlie · 2018-09-30T17:26:54Z

@devotox This looks great. I’ll merge today. I’m on my phone so I can’t merge this in right now, but I’ll do it today for sure.

Dynamoose v1.0 will be released today, and we will get this in for that version.

Due to the fact that I don’t see a super compelling argument for setting the default to true I’m going to leave the default set to false. I’m totally willing to reconsider for another major version. But I’m normally against packages doing too much magic behind the scenes. By default I believe Dynamoose shouldn’t heavily change the behavior of DynamoDB, especially in regards to this. It’s easy for developers to toggle this switch.

For new people to Dynamoose having the default be true might lead to a lot of confusion, which I think it would be good to prevent. Especially coming from the native AWS SDK.

Thanks for all your hard work on this!! I really appreciate it!

devotox · 2018-09-30T21:57:16Z

@fishcharlie You make a lot of sense with that. It should stay as close to the AWS SDK in that sense!

devotox added 2 commits September 5, 2018 11:03

Update expires on updates

170cf31

Update Expires

3c34b68

fishcharlie added the status:do-not-merge label Sep 5, 2018

Update Documentation

c4d9248

fishcharlie requested changes Sep 11, 2018

View reviewed changes

Update Documentation

201bd78

devotox force-pushed the master branch from 42e3858 to 201bd78 Compare September 17, 2018 11:42

Add tests

82a00b6

devotox force-pushed the master branch from a947038 to 82a00b6 Compare September 21, 2018 18:14

fishcharlie requested changes Sep 21, 2018

View reviewed changes

devotox added 2 commits September 24, 2018 10:38

Fixes from Review

7424c98

Fix throughput error

1e91e2a

fishcharlie approved these changes Sep 24, 2018

View reviewed changes

Delete yarn.lock

8b83552

fishcharlie removed the status:do-not-merge label Sep 24, 2018

Merge Conflicts

7b31936

Fix tests

2c7d49d

Merge branch 'master' of https://github.com/dynamoosejs/dynamoose

6487df0

fishcharlie mentioned this pull request Sep 28, 2018

Version 1.0.0 #365

Merged

devotox added 2 commits September 30, 2018 01:58

Merge branch 'master' of https://github.com/dynamoosejs/dynamoose

092095d

Add Update Tests

d13e012

fishcharlie merged commit 7b01a64 into dynamoose:master Sep 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates Expires attribute on Model Updates #420

Updates Expires attribute on Model Updates #420

devotox commented Sep 5, 2018 •

edited by fishcharlie

Loading

coveralls commented Sep 5, 2018 •

edited

Loading

fishcharlie commented Sep 5, 2018

devotox commented Sep 10, 2018

fishcharlie left a comment

fishcharlie Sep 11, 2018

fishcharlie commented Sep 11, 2018

fishcharlie commented Sep 11, 2018

devotox commented Sep 17, 2018 •

edited

Loading

devotox commented Sep 21, 2018

devotox commented Sep 21, 2018

fishcharlie commented Sep 21, 2018

fishcharlie left a comment

fishcharlie Sep 21, 2018

devotox Sep 24, 2018

fishcharlie Sep 24, 2018

fishcharlie Sep 21, 2018

fishcharlie Sep 21, 2018

fishcharlie Sep 21, 2018

fishcharlie left a comment

fishcharlie commented Sep 24, 2018

devotox commented Sep 25, 2018

fishcharlie commented Sep 25, 2018

fishcharlie commented Sep 25, 2018

devotox commented Sep 27, 2018

fishcharlie commented Sep 27, 2018

devotox commented Sep 30, 2018

devotox commented Sep 30, 2018

fishcharlie commented Sep 30, 2018

devotox commented Sep 30, 2018

fishcharlie commented Sep 30, 2018

devotox commented Sep 30, 2018

Updates Expires attribute on Model Updates #420

Updates Expires attribute on Model Updates #420

Conversation

devotox commented Sep 5, 2018 • edited by fishcharlie Loading

Summary:

Type (select 1):

Is this a breaking change? (select 1):

Is this ready to be merged into Dynamoose? (select 1):

Are all the tests currently passing on this PR? (select 1):

Other:

coveralls commented Sep 5, 2018 • edited Loading

Pull Request Test Coverage Report for Build 663

💛 - Coveralls

fishcharlie commented Sep 5, 2018

devotox commented Sep 10, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

fishcharlie Sep 11, 2018

Choose a reason for hiding this comment

fishcharlie commented Sep 11, 2018

fishcharlie commented Sep 11, 2018

devotox commented Sep 17, 2018 • edited Loading

devotox commented Sep 21, 2018

devotox commented Sep 21, 2018

fishcharlie commented Sep 21, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

fishcharlie Sep 21, 2018

Choose a reason for hiding this comment

devotox Sep 24, 2018

Choose a reason for hiding this comment

fishcharlie Sep 24, 2018

Choose a reason for hiding this comment

fishcharlie Sep 21, 2018

Choose a reason for hiding this comment

fishcharlie Sep 21, 2018

Choose a reason for hiding this comment

fishcharlie Sep 21, 2018

Choose a reason for hiding this comment

fishcharlie left a comment

Choose a reason for hiding this comment

fishcharlie commented Sep 24, 2018

devotox commented Sep 25, 2018

fishcharlie commented Sep 25, 2018

fishcharlie commented Sep 25, 2018

devotox commented Sep 27, 2018

fishcharlie commented Sep 27, 2018

devotox commented Sep 30, 2018

devotox commented Sep 30, 2018

fishcharlie commented Sep 30, 2018

devotox commented Sep 30, 2018

fishcharlie commented Sep 30, 2018

devotox commented Sep 30, 2018

devotox commented Sep 5, 2018 •

edited by fishcharlie

Loading

coveralls commented Sep 5, 2018 •

edited

Loading

devotox commented Sep 17, 2018 •

edited

Loading