[4.x] Configure attributes for synced resources when creating models

[abrardev99]

Problem

When a model is being created somewhere, it may need a bit more extra data than what's in the synced attributes. The synced attributes are used for updates, but we want to use different attributes for creating. There are multiple scenarios.

1. Central model is simple, but the tenant model is a little complex (has attributes does not exist in the central model)
2. tenant model is simple, but the central model is complex, then attaching the User to a tenant cause issues.

Must read the connected issue in this PR for more information.

Solution

Note Implementation has been changed. See PR reviews.

This PR enables the ability to specify the attributes in both the central and tenant models. A new method getResourceCreationAttributes added to the Syncable interface which by default returns null.

1. Returning null will copy the resources 1:1.
2. Returning key-value array will be considered the default values and used as it.

public function getResourceCreationAttributes(): array
{
    // Attributes default values when creating resources from tenant to central DB
    return
        [
            'global_id' => 'abc-123',
            'name' => 'John',
            'password' => 'password',
            'email' => 'john@demo',
            'role' => 'admin',
        ];
}

3. Returning an array of attribute names will pick the model attribute values.

public function getResourceCreationAttributes(): array
{
    // Attributes used when creating resources from tenant to central DB
    // Notice here we are not adding "code" filed because it doesn't
    // exist in central model
    return
        [
            'global_id',
            'name',
            'password',
            'email',
            'role'
        ];
}

[stancl]

What are the ci.yml changes here?

[stancl]

Wrote a comment regrading the above on Basecamp 👍🏻

[abrardev99]

@stancl from your review on comments https://github.com/archtechx/tenancy/pull/915#discussion_r941930758 and https://github.com/archtechx/tenancy/pull/915#discussion_r941930690, I realized that we need this method only when creating Central model and we need sync in tenant model (only when creating). In fact, the original issue was also related to creating a model in central DB when creating in tenant DB with the extra column.

In other words, we don't need the getCreateAttributeNames method for the central model. As it implements Syncable, adding method in here enforces to add in tenant model too because SyncMaster is extending from Syncable.

What do you suggest that the developer implement this method only in the tenant model? I can think of a separate interface.

[stancl]

Create a message on BC, we'll discuss it there in depth 👍🏻

[abrardev99]

@stancl I've marked reviews as resolved because new changes change all of the code.

[stancl]

Can you update the PR description?

[abrardev99]

Can you update the PR description?

Done.

[stancl]

Code samples for points 2. and 3. in the README seem to be swapped. Also, please fix formatting and use proper syntax highlighting.

[codecov-commenter]

Codecov Report

Merging #915 (8e664b1) into master (137d80a) will increase coverage by 1.44%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #915      +/-   ##
============================================
+ Coverage     87.16%   88.60%   +1.44%     
- Complexity      455      608     +153     
============================================
  Files           123      132       +9     
  Lines          1262     1694     +432     
============================================
+ Hits           1100     1501     +401     
- Misses          162      193      +31     

Impacted Files	Coverage Δ
src/Database/Concerns/ResourceSyncing.php	100.00% <100.00%> (ø)
src/Events/SyncedResourceSaved.php	100.00% <100.00%> (ø)
src/Listeners/UpdateSyncedResource.php	100.00% <100.00%> (ø)
src/Commands/Install.php	86.00% <0.00%> (-7.75%)	:arrow_down:
src/Middleware/InitializeTenancyByRequestData.php	86.36% <0.00%> (-5.31%)	:arrow_down:
src/Tenancy.php	89.06% <0.00%> (-1.85%)	:arrow_down:
src/Commands/Link.php	86.36% <0.00%> (-0.31%)	:arrow_down:
src/Database/DatabaseConfig.php	91.30% <0.00%> (-0.08%)	:arrow_down:
src/Commands/Up.php	100.00% <0.00%> (ø)
src/Commands/Run.php	100.00% <0.00%> (ø)
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[stancl]

@abrardev99 ready for final review now? And are all of my comments above addressed? Some are now collapsed in the "x hidden comments"

[abrardev99]

ready for final review now? And are all of my comments above addressed? Some are now collapsed in the "x hidden comments."

Yes, all the comments are addressed and PR is ready for final review.

[stancl]

One thing I'm not 100% sure about is that we're using the creation attributes for both the central model and the tenant model, right?

Are the creation attributes defined separately on the central model and the tenant model? Or are we using the same ones for creating the record in both cases?

[abrardev99]

we're using the creation attributes for both the central model and the tenant model, right?

Yes correct.

Are the creation attributes defined separately on the central model and the tenant model?

Yes. If you see a Syncable interface, we have the getSyncedCreationAttributes method. As the Syncable interface is implemented by both models, so creation attributes are defined separately on the central model and the tenant model.

[stancl]

I think that the tests could be a bit simplified.

1. For example, creating the resource in tenant database creates it in central database with attributes names seems to test the behavior that existed before this PR? It tests that the synced attribute names are used, but I don't see you making any assertions about unsynced attributes that are part of creation attributes being included on the created model
2. It seems that each test has two variants? 1) central -> tenant, 2) tenant ->central. Is there any way we could avoid the duplication, perhaps with something like datasets. Only do this if it can improve the tests. If they were to become more complex with datasets, it's not worth making this change. But try to find other ways to simplify this. Perhaps we could assert that the logic works correctly in both directions in a single test. So there'd be a test for a list of attribute names used for creation, and it'd assert that this works in both directions. Then there'd be a test for a mix of attribute names and default values and it'd assert that it works in both directions. Etc. Try to find something like that to avoid the duplication. We ideally only want one test for each case (attribute names, default values, mix of attribute names and default values, etc).
3. I'd improve the terminology used in the test names, e.g. the attribute names in test names are a bit confusing, but if you can't do this I can make the changes after you clean up the tests.

[stancl]

Just a note for future reference: the recent commits add behavior for excluding the model key from the attributes used for creating the model in other contexts — https://3.basecamp.com/5170965/buckets/23650968/chats/4064741444@5394258122

[stancl]

A note about code style — try to write code that follows our code style on your own, don't depend on php-cs-fixer fixing your code. It should just catch edge cases. We don't use it in tests, but tests should still follow our code style. Basic things like spaces in function () {, class Foo having { on the next line, etc.

[stancl]

@abrardev99 There's still this:

Note: Implementation has been changed. See PR reviews. I'll update the description when resolved all the reviews.

in the PR description. Make the PR description up to date since we're about to merge it.

[abrardev99]

@abrardev99 There's still this:

Note: Implementation has been changed. See PR reviews. I'll update the description when resolved all the reviews.

in the PR description. Make the PR description up to date since we're about to merge it.

Forgot to remove it. Will update it.

[abrardev99]

A note about code style — try to write code that follows our code style on your own, don't depend on php-cs-fixer fixing your code. It should just catch edge cases. We don't use it in tests, but tests should still follow our code style. Basic things like spaces in function () {, class Foo having { on the next line, etc.

Noted. I do try to follow standards but sometimes miss especially when there is a lot of code.

[stancl]

Thanks for working on this PR! Happy to have it finally merged 🚀

After making the small tweaks I described above, you can work on these two tasks that are related to resource syncing:

1. https://3.basecamp.com/5170965/buckets/23651018/todos/5496514869
2. https://3.basecamp.com/5170965/buckets/23651018/todos/5426057990

The first one is very simple so it shouldn't take more than 30 minutes I think. The second one is more complex so that's what you can work on for the rest of the day 👍