john-bodley commented on code in PR #25031:
URL: https://github.com/apache/superset/pull/25031#discussion_r1300551674
##########
superset/models/dashboard.py:
##########
@@ -67,17 +67,19 @@
logger = logging.getLogger(__name__)
-def copy_dashboard(_mapper: Mapper, connection: Connection, target: Dashboard)
-> None:
+def copy_dashboard( # pylint: disable=unused-argument
+ _mapper: Mapper,
+ connection: Connection,
+ target: Dashboard,
+) -> None:
dashboard_id = config["DASHBOARD_TEMPLATE_ID"]
if dashboard_id is None:
return
- session_class = sessionmaker(autoflush=False)
- session = session_class(bind=connection)
- new_user = session.query(User).filter_by(id=target.id).first()
+ new_user = db.session.query(User).filter_by(id=target.id).first()
# copy template dashboard to user
- template = session.query(Dashboard).filter_by(id=int(dashboard_id)).first()
+ template =
db.session.query(Dashboard).filter_by(id=int(dashboard_id)).first()
Review Comment:
@michael-s-molina I've reverted the PR back to my initial implementation
which leverages the `connection` parameter. It states that it provides a handle
into the current transaction, yet we're likely over committing, i.e., there
should only be one commit per unit of work. I'm not sure what happens if you
close the session without committing. Maybe we using the connection wrongly.
##########
superset/models/dashboard.py:
##########
@@ -67,17 +67,19 @@
logger = logging.getLogger(__name__)
-def copy_dashboard(_mapper: Mapper, connection: Connection, target: Dashboard)
-> None:
+def copy_dashboard( # pylint: disable=unused-argument
+ _mapper: Mapper,
+ connection: Connection,
+ target: Dashboard,
+) -> None:
dashboard_id = config["DASHBOARD_TEMPLATE_ID"]
if dashboard_id is None:
return
- session_class = sessionmaker(autoflush=False)
- session = session_class(bind=connection)
- new_user = session.query(User).filter_by(id=target.id).first()
+ new_user = db.session.query(User).filter_by(id=target.id).first()
# copy template dashboard to user
- template = session.query(Dashboard).filter_by(id=int(dashboard_id)).first()
+ template =
db.session.query(Dashboard).filter_by(id=int(dashboard_id)).first()
Review Comment:
@michael-s-molina I've reverted the PR back to my initial implementation
which leverages the `connection` parameter.
It states that it provides a handle into the current transaction, yet we're
likely over committing, i.e., there should only be one commit per unit of work.
I'm not sure what happens if you close the session without committing. Maybe
we using the connection wrongly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
