Change: when forking a query, copy all visualizations #1256
Conversation
| @@ -715,6 +715,32 @@ def recent(cls, groups, user_id=None, limit=20): | |||
|
|
|||
| return query | |||
|
|
|||
| @classmethod | |||
| def fork(cls, id, user, org): | |||
There was a problem hiding this comment.
Why class method and not an instance method?
There was a problem hiding this comment.
I think that fork logic (fetch query and chart data by query id, then change their name and id, then insert them) should be model's method.
There was a problem hiding this comment.
Anyway the fork method shouldn't fetch the query. It will either operate on itself (if it's an instance method) or on a given query (if we keep it as a class method).
What benefit we have from having this as a class method?
There was a problem hiding this comment.
ok, change instance methdo
arikfr
changed the title
|
@ninneko have you seen my comments? |
|
Sorry, I was not able to commit because I have been sick. |
|
Sorry to hear that. I hope you're feeling better now! No pressure with the pull request. Just wanted to make sure you noticed the Thanks. On Sep 15, 2016 11:01 AM, "yohei.naruse" [email protected] wrote:
|
ninneko
force-pushed
the
fork_with_vis
branch
from
7fc8597
to
7bc71c9
Compare
|
@ninneko you need to update the tests, because they still use the class method instead of instance method. |
Change: when forking a query, copy all visualizations
Closes #439.