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

Add multi tenancy capabilities with ecto meta prefix #25

Conversation

dreamingechoes
Copy link
Contributor

Connects to #24.

What does this PR do?

Adds multi tenancy capabilities leveraging the Ecto.Query prefix.

Changes made in lib/paper_trail.ex

A new private function add_prefix was added into lib/paper_trail.ex in order to
add the new :prefix option into the Version changeset and be able to store the data
on the tenant specified by the user.

Removes the unnecessary functions get_version and get_versions in lib/paper_trail.ex
and delegate them to the PaperTrail.VersionQueries module.

Changes made in lib/paper_trail/version_queries.ex

The version_query private function was redefined to allow the addition of the options
specified (only :prefix at this moment) on the filter query.

Some new definitions of the main functions were added to allow the new option parameter,
as well as some specs and basic documentation.

Other changes

Documentation of the README.md file was updated with some information about this
new capability.

Where should the reviewer start?

Checking the new code added into lib/paper_trail.ex and lib/paper_trail/version_queries.ex.

This new function will allow the user to add information to the Ecto 
meta :prefix in order to use a different tenant for the operation on
the version struct.
This allows to remove unnecessary functions from PaperTrail module
Includes new definitions in order to be able to pass a list of options
to the final query and be able to set data such as Ecto :prefix. This adds
some basic documentation as well.
@izelnakri
Copy link
Owner

izelnakri commented Jul 29, 2017

Thank you very much for such a high-quality pull request! I do not intend to support mysql however I can see that prefix feature could still be useful for postgres. Additionaly thanks for the documentation and code improvements.

If you could write a basic test case with two prefixed queries and assert that those two produce different results then I can merge this pull request. We should run at least one test for this feature, so that it wont break somehow in future releases.

Thanks again for your contribution, feel free to add yourself to credits part as well ;)

@dreamingechoes
Copy link
Contributor Author

Hi @izelnakri!

I'm glad to see that you found this PR interesting. I was waiting for your feedback to know if you agree with the changes I made and if I would have to make something else before add some tests for this, so I'll add them tomorrow so you can review the PR again and decide if you want to merge this.

Thank you so much for your awesome work with this package.

Greetings! 😄

This module contains functions related with multi tenancy for test purposes
This module contains functions related with multi tenancy for test purposes
This module contains functions related with models queries for test purposes
This module contains functions related with models changesets for test purposes
Includes some new functions as well with common behaviors
Includes some new functions as well with common behaviors
@dreamingechoes
Copy link
Contributor Author

Hi @izelnakri!

I just added some tests (in the example application tests as well as in the library ones) based on the original tests, so you can review it easily (or at least I hope so).

Notice that to see the example on green, it's required to change the reference of the paper_trail package in the example/mix.exs file to the proper one, something like:

{:paper_trail, git: "https://github.com/dreamingechoes/paper_trail", branch: "add-multi-tenancy-capabilities-with-ecto-meta-prefix"}

Tell me if you see something weird in order to change it before merging the PR 😄

@@ -0,0 +1,24 @@
defmodule ChangesetHelper do
Copy link
Owner

@izelnakri izelnakri Aug 5, 2017

Choose a reason for hiding this comment

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

I'm sorry to ask for this, but I think this abstraction makes the tests hard to read with very little benefit, could you remove this abstraction and change the tests?

@@ -0,0 +1,24 @@
defmodule QueryHelper do
Copy link
Owner

@izelnakri izelnakri Aug 5, 2017

Choose a reason for hiding this comment

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

I'm sorry to ask for this, but I think this abstraction makes the tests hard to read with very little benefit, could you remove this abstraction and change the tests?

…pport

Now the aux functions used on the example app tests are located in each
test file in stead of in one separated module.
@dreamingechoes
Copy link
Contributor Author

Hi @izelnakri!

You're right. I have created those modules in order to place the aux functions used in the example app tests and avoid the repetition of code, but it's true that this could make the tests harder to understand, so I just removed the extra modules and I have placed the aux functions right at the end of the test files so it could be more understandable.

Please if you have any other suggestion tell me, so I can make the changes and push them asap.

Thank you! 😄

Copy link
Owner

@izelnakri izelnakri left a comment

Choose a reason for hiding this comment

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

It mostly looks good Im going to do a final review soon.

# Company related functions
def company_count(), do: (from company in Company, select: count(company.id))
def first_company(), do: (first(Company, :id) |> preload(:people))
def new_company(attrs), do: Company.changeset(%Company{}, attrs)
Copy link
Owner

Choose a reason for hiding this comment

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

new_company and updated_company functions could be removed for more explicitness. I think Company.changeset(company, attrs) is better than update_company(company, attrs)


:ok
end

test "creating a person with meta tag creates a person version with correct attributes" do
company = first(Company, :id) |> Repo.one
test "[multi tenant] creating a person with meta tag creates a person version with correct attributes" do
Copy link
Owner

Choose a reason for hiding this comment

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

this doesnt test mulit-tenancy?

Copy link
Owner

Choose a reason for hiding this comment

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

We are infact never testing multi tenancy in this file as far as I see, (Im not suggesting that we should)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Omg, this is embarrassing 😳 It's a typo due some "copy/paste" on the test descriptions, I'll remove this.

Removes all the aux functions in order to make all the variables more
explicit and easier to understand. Removes some typos in person_test 
description.
# Multi tenant tests
test "[multi tenant] count works" do
versions = add_three_versions(MultiTenant.tenant())
Version.count(prefix: MultiTenant.tenant()) == length(versions)
Copy link
Owner

Choose a reason for hiding this comment

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

we should also add that Version.count() != length(versions)

Copy link
Owner

@izelnakri izelnakri left a comment

Choose a reason for hiding this comment

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

General idea is that we should also assert that the prefix option actually makes a difference, that means there shouldnt be any Version in the public schema of postgres if we use another schema as our target. I commented here only few of those possible assertions, you can fill the remaining.

There are many tests here are copy/paste/duplicated from the original tests, normally we wouldn't need to have them and test the absolute minimal functionality. However I do mention that we assail paper_trail with tests in the README so Im fine with having them :)

I think I can merge this once we have the remaining assertions! Well done


test "[multi tenant] first works" do
add_three_versions(MultiTenant.tenant())
Version.first(prefix: MultiTenant.tenant()) |> serialize == @valid_attrs
Copy link
Owner

Choose a reason for hiding this comment

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

we should also add: Version.first() |> serialize != @valid_attrs

item_changes: %{first_name: "Yukihiro", last_name: "Matsumoto"},
origin: "test",
inserted_at: DateTime.from_naive!(~N[1965-04-14 01:00:00.000], "Etc/UTC")
}
Copy link
Owner

Choose a reason for hiding this comment

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

we should also add: Version.last() == nil


assert PaperTrail.get_version(last_person) == target_version
assert PaperTrail.get_version(Person, last_person.id) == target_version
assert PaperTrail.get_version(last_person_multi, prefix: tenant) == target_version_multi
assert PaperTrail.get_version(Person, last_person_multi.id, prefix: tenant) == target_version_multi
Copy link
Owner

Choose a reason for hiding this comment

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

also target_version != target_version_multi


assert PaperTrail.get_versions(last_person) == target_versions
assert PaperTrail.get_versions(Person, last_person.id) == target_versions
assert PaperTrail.get_versions(last_person_multi, prefix: tenant) == target_versions_multi
assert PaperTrail.get_versions(Person, last_person_multi.id, prefix: tenant) == target_versions_multi
Copy link
Owner

Choose a reason for hiding this comment

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

also target_version != target_version_multi

origin: "admin",
meta: %{"linkname" => "izelnakri"}
}
assert deleted_person |> serialize == person_before_deletion |> serialize
Copy link
Owner

Choose a reason for hiding this comment

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

also assert that Version.count() == 0

origin: "scraper",
meta: %{"linkname" => "izelnakri"}
}
assert person == first_person(:multitenant) |> serialize
Copy link
Owner

Choose a reason for hiding this comment

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

also assert that Version.count() == 0


assert company_count == [1]
assert version_count == [1]

Copy link
Owner

Choose a reason for hiding this comment

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

Version.count == [0]


assert company_count == [1]
assert version_count == [2]

Copy link
Owner

Choose a reason for hiding this comment

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

assert Version.count == [0]


assert company_count == [0]
assert version_count == [3]

Copy link
Owner

Choose a reason for hiding this comment

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

assert Version.count == [0]

This will allow us to delete records with on cascade relations in the
same schema (it’s not possible at this moment).
This additional assertions will allow us to compare results between regular
behavior and the multi tenant one, and check if multi tenant generates some
conflicts on the public schema/database.
@izelnakri izelnakri merged commit 6fec5cf into izelnakri:master Aug 12, 2017
@izelnakri
Copy link
Owner

Fantastic work, thank you very much!

@dreamingechoes
Copy link
Contributor Author

Thanks to you @izelnakri! I'm glad to contribute to this package. Thank you for your work and your awesome feedback during the pull request 😄

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