called syncOriginal() after setting attributes getAll() and find() - … #58

zoul0813 · 2017-06-22T16:24:37Z

added support for timestamps, and fixed "isDirty" logic (isDirty is required for timestamps to work properly).

This change is

…this fixes the 'dirty' issue, support timestamps (dirty needs to work for timestamps to function properly for updated_at)

baopham · 2017-06-22T16:37:41Z

Nice. This change would need some unit tests to make sure it is working and won't break in the future.

If you need help with the tests, let me know. It'd be nice if we can support timestamps.

zoul0813 · 2017-06-22T20:50:34Z

Having trouble getting DynamoDbLocal setup for the test.

I already have DDb Local setup for my laravel project, and use the following to start it ...

java -Djava.library.path=..path/DynamoDBLocal_lib -jar ..path/DynamoDBLocal.jar -sharedDb --port 8081 -dbPath ..path/webroot/database/local/dynamo

I adjusted this to the following for the tests

java -Djava.library.path=..path/DynamoDBLocal_lib -jar ..path/DynamoDBLocal.jar - -port 3000 -dbPath ..path/tmp/laravel-dynamodb/ (my fork is in the tmp folder)

Doing this, I managed to go from having every test generate errors (and take forever trying to connect to what was not there), and now am getting 6 errors ... they all appear to be the same error though.

5) BaoPham\DynamoDb\Tests\DynamoDbModelTest::testUpdateRecord
Error: Call to a member function connection() on null

...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Model.php:1042
...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Model.php:1008
...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Concerns/HasAttributes.php:777
...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Concerns/HasAttributes.php:732
...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Concerns/HasAttributes.php:525
...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Model.php:1285
...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Concerns/HasTimestamps.php:71
...path/tmp/laravel-dynamodb/vendor/illuminate/database/Eloquent/Concerns/HasTimestamps.php:42
...path/tmp/laravel-dynamodb/src/DynamoDbModel.php:127
...path/tmp/laravel-dynamodb/src/DynamoDbModel.php:147
...path/tmp/laravel-dynamodb/tests/DynamoDbModelTest.php:67

Do I need to do something other than start DDb Local pointing at the project directory? Looks like the tests take the dynamodb_local_init.db and move it to dynamodb_local_test.db which I assume creates all of the necessary tables, with expected default items in it?

baopham · 2017-06-23T04:06:41Z

the tests take the dynamodb_local_init.db and move it to dynamodb_local_test.db which I assume creates all of the necessary tables

Yes

Can you make sure you cd into the project root and run the exact command:

java -Djava.library.path=./DynamoDBLocal_lib -jar dynamodb_local/DynamoDBLocal.jar --port 3000

Then in a new tab, run:

./vendor/bin/phpunit

…CONTRIBUTING.md with instructions to start DynamoDB Local and PHPUnit

zoul0813 · 2017-06-23T13:30:47Z

Ahh, looks like I actually did have it all setup correctly. Following the errors, the timestamp logic does apparently try to connect to the database at some point to retrieve a date/time format in the event that one is not already set on the model. Should be an easy fix to explicitly set the date/time format on DynamoDbModel - just a matter of picking a default format.

I'm thinking we should use the format generated by JavaScript's Date toJSON method, as described here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toJSON

baopham · 2017-06-23T13:40:41Z

Since Laravel is using Carbon so why not use its toIso8601String http://carbon.nesbot.com/docs/#api-commonformats?

zoul0813 · 2017-06-23T13:44:28Z

I figured using the JavaScript json format would make the json document more compatible with other application code that may be using/accessing it (lambda triggers, client side code, mobile apps, etc) either directly or indirectly.

zoul0813 · 2017-06-23T13:47:45Z

Looks like those two formats are the same actually. Javascripts date object calls toIsoString behind the scenes. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString

baopham · 2017-06-23T13:48:55Z

ISO 8601 format it is then 👍

zoul0813 · 2017-06-23T13:54:04Z

Adding protected $dateFormat = 'o'; to DynamoDbModel seems to have solved the problem.

> phpunit
PHPUnit 5.7.21 by Sebastian Bergmann and contributors.

........................................................          56 / 56 (100%)

Time: 1.84 seconds, Memory: 32.00MB

OK (56 tests, 122 assertions)

baopham · 2017-06-23T13:57:29Z

Ah yeah totally forgot about that!

Still need to have 2 test cases though, one for created_at and one for updated_at.

# The "O" or "o" standard format specifier represents a custom date and time format string using a pattern that preserves time zone information and emits a result string that complies with ISO 8601. # https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-date-and-time-format-strings#Roundtrip

zoul0813 · 2017-06-23T14:38:00Z

Not quite sure why, but all the timestamps are being set to 1970-01-01 ... in my app, they are being set to the correct current time. I checked the HasTimestamps trait and verified that it's creating a new Carbon instance, with the current time ... but for some reason, after it's being set to $this->testModel->created_at it gets set to 1970-01-01 ... not quite sure where this is happening - any ideas? I'm thinking it may have something to do with the tests themselves?

-- Nevermind, I think it has to do with the format I'm using ... working on it now.

baopham · 2017-06-23T14:57:46Z

Let me try and see

baopham · 2017-06-23T15:05:03Z

For a more deterministic test, maybe we should try Carbon::setTestNow

Nitpick-CI · 2017-06-23T15:07:13Z

tests/DynamoDbTimestampTest.php

+        return $item;
+    }
+
+}


The closing brace for the class must go on the next line after the body

Nitpick-CI · 2017-06-23T15:07:14Z

tests/DynamoDbTimestampTest.php

+
+}
+
+class TimestampModel extends \BaoPham\DynamoDb\DynamoDbModel


Each class must be in a file by itself

Nitpick is really nit-picky... can I just ignore some of these?

Haha yeah, this one can be ignored.

Nitpick-CI · 2017-06-23T15:07:14Z

tests/DynamoDbTimestampTest.php

+            'hash' => 'count',
+        ],
+    ];
+}


Expected 1 blank line at end of file; 2 found

zoul0813 · 2017-06-23T15:09:50Z

I created a new test class, so we can setup as many tests as needed - this also allows the other test classes to set public $timestamps = false;, as timestamps are on by default and may effect the other test results.

I did use Carbon::setTestNow()

The problem was using protected $dateFormat = 'o';, I wasn't paying attention and got that shorthand format off some Microsoft site ... I switched it over to DateTime::ISO8601 from the PHP Docs...

baopham · 2017-06-23T15:12:48Z

composer.json

+    "scripts": {
+        "test": "phpunit",
+        "dynamodb_local": "java -Djava.library.path=./DynamoDBLocal_lib -jar dynamodb_local/DynamoDBLocal.jar --port 3000"
+    },


Very nice! just like package.json 😄

baopham · 2017-06-23T15:12:58Z

CONTRIBUTING.md

+
+or
+
+```composer run-script tests```


Do you mean test here?

I did ... fixing now

baopham · 2017-06-23T15:13:33Z

CONTRIBUTING.md

@@ -1,3 +1,19 @@
 # Contribution Guide

 Thank you for considering contributing to this library. Please make sure your code follows the PSR-2 coding standard and the PSR-4 autoloading standard before sending a pull request.
+
+## Running Tests


Might as well remove the test section in README.

Ahh, didn't realize there was a test section there - did you add it recently?

No it's been there for a while :)

I updated CONTRIBUTING.md and README.md

baopham · 2017-06-23T15:14:26Z

tests/DynamoDbTimestampTest.php

+
+    public function testCreateRecord()
+    {
+        \Carbon\Carbon::setTestNow(\Carbon\Carbon::create(2017, 06, 24, 5, 30, 0));


Maybe put a use Carbon\Carbon on top?

yeah, ... that would make sense :)

Nitpick-CI · 2017-06-23T15:19:27Z

tests/DynamoDbTimestampTest.php

+    }
+}
+
+class TimestampModel extends \BaoPham\DynamoDb\DynamoDbModel


Each class must be in a file by itself

…ch README.md style (- and = instead of #), added DynamoDb Local details to CONTRIBUTING.md

baopham · 2017-06-23T15:59:46Z

Many thanks @zoul0813 for pushing this through.

zoul0813 · 2017-06-23T16:04:30Z

Your welcome, and don't forget to check off the TODO item at the bottom of README.md ... just saw that.

baopham · 2017-06-23T16:06:32Z

Yep one down!

called syncOriginal() after setting attributes getAll() and find() - …

bb8dace

…this fixes the 'dirty' issue, support timestamps (dirty needs to work for timestamps to function properly for updated_at)

This was referenced Jun 22, 2017

support timestamps #57

Closed

isDirty() after retrieving models #51

Closed

Added composer.json scripts for 'test' and 'dynamodb_local', updated …

6d1b363

…CONTRIBUTING.md with instructions to start DynamoDB Local and PHPUnit

zoul0813 added 2 commits June 23, 2017 11:06

setup proper date format

4ab4482

added new test class for timestamps

3478f08

Nitpick-CI reviewed Jun 23, 2017

View reviewed changes

tests/DynamoDbTimestampTest.php Outdated

return $item;

}

}

Copy link

Nitpick-CI Jun 23, 2017

Choose a reason for hiding this comment

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

The closing brace for the class must go on the next line after the body

Nitpick-CI reviewed Jun 23, 2017

View reviewed changes

tests/DynamoDbTimestampTest.php

'hash' => 'count',

],

];

}

Copy link

Nitpick-CI Jun 23, 2017

Choose a reason for hiding this comment

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

Expected 1 blank line at end of file; 2 found

baopham reviewed Jun 23, 2017

View reviewed changes

nitpick

82de0bf

Nitpick-CI reviewed Jun 23, 2017

View reviewed changes

zoul0813 added 3 commits June 23, 2017 11:20

use Carbon\Carbon;

86e6614

fixed "test"

97fe448

removed Test from README.md, updated format of CONTRIBUTING.md to mat…

d17ded1

…ch README.md style (- and = instead of #), added DynamoDb Local details to CONTRIBUTING.md

baopham merged commit bd940d3 into baopham:master Jun 23, 2017

zoul0813 deleted the feature/dirty-timestamps branch June 23, 2017 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

called syncOriginal() after setting attributes getAll() and find() - … #58

called syncOriginal() after setting attributes getAll() and find() - … #58

zoul0813 commented Jun 22, 2017 •

edited by baopham

baopham commented Jun 22, 2017

zoul0813 commented Jun 22, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017 •

edited

baopham commented Jun 23, 2017

baopham commented Jun 23, 2017

Nitpick-CI Jun 23, 2017

Nitpick-CI Jun 23, 2017

zoul0813 Jun 23, 2017

baopham Jun 23, 2017

Nitpick-CI Jun 23, 2017

zoul0813 commented Jun 23, 2017

baopham Jun 23, 2017

zoul0813 Jun 23, 2017

baopham Jun 23, 2017

zoul0813 Jun 23, 2017

baopham Jun 23, 2017

zoul0813 Jun 23, 2017

baopham Jun 23, 2017

zoul0813 Jun 23, 2017

baopham Jun 23, 2017

zoul0813 Jun 23, 2017

Nitpick-CI Jun 23, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017 •

edited

baopham commented Jun 23, 2017


		}

		class TimestampModel extends \BaoPham\DynamoDb\DynamoDbModel

called syncOriginal() after setting attributes getAll() and find() - … #58

called syncOriginal() after setting attributes getAll() and find() - … #58

Conversation

zoul0813 commented Jun 22, 2017 • edited by baopham

baopham commented Jun 22, 2017

zoul0813 commented Jun 22, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017 • edited

baopham commented Jun 23, 2017

baopham commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoul0813 commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baopham commented Jun 23, 2017

zoul0813 commented Jun 23, 2017 • edited

baopham commented Jun 23, 2017

zoul0813 commented Jun 22, 2017 •

edited by baopham

zoul0813 commented Jun 23, 2017 •

edited

zoul0813 commented Jun 23, 2017 •

edited