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

Big cleanup in the join logic and addition of big tests #107

Merged
merged 120 commits into from
Aug 26, 2016

Conversation

romaninsh
Copy link
Member

@romaninsh romaninsh commented Aug 19, 2016

This PR introduces few necessary tools to run more sophisticated tests and then fixes a few issues that were spotted when using Agile Data in commercial projects.

SMBO TestSuite

SMBO is an accounting app. A few models have been mocked up with the similar logic and presented in tests/smbo/lib. Test scripts use those models to test a deeper logic.

Added TransferTest

In SMBO there is an entity called Document. Any document with doc_type='payment' is considered Payment. Some of those payments can be Transfers.

Transfers exist in pairs - one transfer decrease amount on source account, the other transfer increases the amount on the destination account.

Creating a transfer will create the other leg too and cross-link those documents.

Implementing Cloning support.

When transfer document is created and we are willing to create a destination record, it is actually mirror-matched to our own record. Although we could copy all the properties, some users may want to do a "clone" here.

I have improved support for model cloning. The new clone must have no associations with the source model. Previously something wasn't linking up correctly, so when creating a transfer, some ID values were shared incorrectly.

Join refactoring

Previously joins were linked between 2 physical fields. When creating a join they were updating "insert" query of the parent to add the field in there.

Now joins are refactored in a way where they are joined between a Model field and a physical field of joined table. So in theory you should be able to:

$this->getRef('user_id')->addField('user_county_id', 'country_id');
$this->join('country', 'user_country_id');

Instead of beforeInsertQuery it now uses beforeInsert and same thing with modification.

foreach ($this->elements as $id => $el) {
if ($el instanceof Join) {
$this->elements[$id] = clone $el;
$el->owner = $this;
Copy link
Member

Choose a reason for hiding this comment

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

I guess there is bug. $el variable will be lost after foreach anyway. Probably $this->elements[$id]->owner should be there instead of $el->owner.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are correct!

Copy link
Member Author

Choose a reason for hiding this comment

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

strangely - changing this logic breaks the build.

Copy link
Member

Choose a reason for hiding this comment

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

it shouldn't be like that

@romaninsh romaninsh mentioned this pull request Aug 22, 2016
3 tasks
@romaninsh romaninsh added this to the 1.1.0 milestone Aug 22, 2016
@romaninsh romaninsh merged commit bb0b5e8 into develop Aug 26, 2016
@romaninsh romaninsh deleted the feature/major-join-cleanup branch August 26, 2016 08:53

### Introducing Actions

ORM/Active Record is not designed for relational databases. There are too much potential that ORM
Copy link
Member

Choose a reason for hiding this comment

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

should be "... is not only designed ..." instead of "... is not designed ..." otherwise reader will think that AD will not work for SQL bases at all :)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, valid point.

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.

2 participants