-
Notifications
You must be signed in to change notification settings - Fork 652
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
2660 extract storage take4 #2775
Conversation
Frontend tests were OK 👍 (details) |
@@ -217,7 +217,7 @@ def public_map | |||
@user_domain = user_domain_variable(request) | |||
|
|||
@public_tables_count = @visualization.user.public_table_count | |||
@nonpublic_tables_count = @related_tables.select{|p| p.privacy != ::Table::PRIVACY_PUBLIC }.count | |||
@nonpublic_tables_count = @related_tables.select{|p| p.privacy != ::UserTable::PRIVACY_PUBLIC }.count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you forgot changing this check as you've done in the following lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d6c3efa
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
retest this, please |
Frontend tests were OK 👍 (details) |
retest this, please |
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
2660 extract storage take4 [in testing]
Another version, without access to user_table.
table.user_table.*
is no longer permitted.This was done to avoid exposing functionality of the model through the
Table
service/decorator/façade, as Nacho suggested. Why? I tried two different approaches:Table
. Almost imposible because of the richness of theSequel::Model
,Sequel::Dataset
and related classes.UserTable
as dumb and ignorant as possible. Not fully possible, cause many business rules are implemented through hooks.UserTable
functionality directly to client classes. Although it sounded like a good idea at first, this way it is hard to enforce certain business rules (such as setting a table name) without a complex interaction betweenTable
andUserTable
.So finally this is the best design I could come up having something working.
If I were to refactor this again, I'd probably take a different approach: do not try to fully isolate the model but extract pieces of functionality little by little and assuming the
Model
would still be the "director" of the interactions. Start with value objects and continue with a UserData service or similar.Cc'ing @Kartones @juanignaciosl and @javisantana so that you have something funny to review when you get to work on Monday :)
Have a nice weekend!