[DRAFT]- Elizabeth/cord poc #605

eschutho · 2023-09-13T17:23:48Z

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

…is (apache#20627)" This reverts commit 9362e27.

eschutho · 2023-09-13T17:24:12Z

superset/message_threads/models.py

+class MessageThread(Model):
+    table_name = "message_threads"
+    id = Column(Integer, primary_key=True)
+    user_id = Column(Integer, ForeignKey("ab_user.id"), nullable=False)


remove this

Suggested change

user_id = Column(Integer, ForeignKey("ab_user.id"), nullable=False)

github-actions

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

codecov-commenter · 2023-09-13T17:24:51Z

Codecov Report

Patch coverage: 65.94% and project coverage change: -10.39% ⚠️

Comparison is base (3630d68) 68.89% compared to head (7ce6542) 58.50%.
Report is 77 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #605       +/-   ##
===========================================
- Coverage   68.89%   58.50%   -10.39%     
===========================================
  Files        1906     1909        +3     
  Lines       74628    74762      +134     
  Branches     8208     8206        -2     
===========================================
- Hits        51412    43742     -7670     
- Misses      21086    28891     +7805     
+ Partials     2130     2129        -1

Flag	Coverage Δ
hive	`53.82% <65.94%> (+0.04%)`	⬆️
mysql	`?`
postgres	`?`
presto	`53.72% <65.94%> (+0.04%)`	⬆️
python	`61.29% <65.94%> (-21.59%)`	⬇️
sqlite	`?`
unit	`55.12% <65.94%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...dashboard/components/SliceHeaderControls/index.tsx	`71.57% <ø> (+1.17%)`	⬆️
...rontend/src/dashboard/containers/DashboardPage.tsx	`8.69% <ø> (ø)`
superset/message_threads/commands/create.py	`44.00% <44.00%> (ø)`
superset/dashboards/api.py	`50.61% <47.05%> (-42.00%)`	⬇️
superset/daos/thread.py	`70.73% <70.73%> (ø)`
superset/message_threads/models.py	`72.22% <72.22%> (ø)`
superset/message_threads/api.py	`80.00% <80.00%> (ø)`
superset/initialization/__init__.py	`91.15% <100.00%> (+0.05%)`	⬆️

... and 293 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eschutho · 2023-09-13T17:25:06Z

superset/migrations/versions/2023-08-25_16-27_6dadc1a73f3a_create_threads.py

+    op.create_table(
+        "message_threads",
+        sa.Column("id", sa.Integer(), nullable=False),
+        sa.Column("user_id", sa.Integer(), nullable=False),


Suggested change

sa.Column("user_id", sa.Integer(), nullable=False),

eschutho · 2023-09-13T17:27:25Z

superset/message_threads/commands/create.py

+                    chart_id=self._chart_id,
+                    dashboard_id=self._dashboard_id,
+                )
+                return thread


we may need to return a flag if this is new so that the client side knows to generate a new thread on the cord side.

eschutho · 2023-09-26T23:29:23Z

superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx

@@ -373,6 +424,29 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => {
    ? t('Exit fullscreen')
    : t('Enter fullscreen');

+  const [threadListId, setthreadListId] = useState<string | null>(null);
+  const [threadIsOpen, setThreadIsOpen] = useState<boolean>(false);
+  const [selectedThreadID, setSelectedThreadID] = useState<string | null>(null);


@kgabryje I had originally intended for this UUID to be for a thread, not a threadList, so that's why the naming is a bit wonky.

eschutho · 2023-09-26T23:31:32Z

superset/cord_auth_token/commands/create.py

+        default=lambda o: {key: value for key, value in o.__dict__.items() if value},
+        indent=4,
+        allow_nan=False,
+    )


the above code is all from Cord's python sdk file. We may not need it, but I left it here in case you wanted it for anything.

eschutho · 2023-09-26T23:32:00Z

superset/cord_auth_token/commands/create.py

+            # the existing auth_token if it exists
+            return get_client_auth_token(
+                app_id="<todo>",
+                secret="<todo>",


you'll want to update this on your local implementation.

eschutho · 2023-09-26T23:33:33Z

superset/cord_auth_token/models.py

+    user_uuid = Column(String(255), nullable=False, default=uuid.uuid4)
+    expires_at = Column(Integer, nullable=False)
+
+    def __init__(self, user_id, workspace_name, auth_token, expires_at, user_uuid):


this doesn't persist to the db, but is used in just setting up an instance of the model for the api

eschutho · 2023-09-26T23:34:17Z

superset/daos/message_thread.py

+logger = logging.getLogger(__name__)
+
+
+class MessageThreadDAO(BaseDAO[MessageThread]):


these MessageThreads are actually all going to be ThreadList components now.. this was my earlier naming. @Antonio-RiveroMartnez

eschutho · 2023-09-26T23:34:44Z

superset/daos/message_thread.py

+
+        # not sure if we also need this check
+        dashboard = (
+            db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).first()


per the discussion, not sure if we want to persist this or not.

eschutho · 2023-09-26T23:36:53Z

superset/message_threads/models.py

+    dashboard_id = Column(Integer, ForeignKey("dashboards.id"), nullable=True)
+    chart_id = Column(Integer, ForeignKey("slices.id"), nullable=True)
+    uuid = Column(String(36), unique=True, nullable=False, default=uuid.uuid4)
+    workspace_name = Column(String(255), nullable=False)


we don't need workspace name any more

eschutho added 2 commits August 15, 2023 16:25

Revert "fix(chart): Time Series set showMaxLabel as null for time xAx…

653a403

…is (apache#20627)" This reverts commit 9362e27.

Merge branch 'master' of github.com:preset-io/superset

32af707

eschutho commented Sep 13, 2023

View reviewed changes

github-actions bot reviewed Sep 13, 2023

View reviewed changes

eschutho commented Sep 13, 2023

View reviewed changes

eschutho force-pushed the elizabeth/cord-poc branch from 7ce6542 to dbbfda5 Compare September 26, 2023 23:26

eschutho commented Sep 26, 2023

View reviewed changes

initial commit

8795ab4

eschutho force-pushed the elizabeth/cord-poc branch from dbbfda5 to 8795ab4 Compare September 26, 2023 23:38

eschutho closed this Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT]- Elizabeth/cord poc #605

[DRAFT]- Elizabeth/cord poc #605

eschutho commented Sep 13, 2023

eschutho Sep 13, 2023

eschutho Sep 13, 2023

github-actions bot left a comment

codecov-commenter commented Sep 13, 2023

eschutho Sep 13, 2023

eschutho Sep 13, 2023

eschutho Sep 26, 2023 •

edited

Loading

eschutho Sep 26, 2023

eschutho Sep 26, 2023

eschutho Sep 26, 2023

eschutho Sep 26, 2023

eschutho Sep 26, 2023

eschutho Sep 26, 2023

		logger = logging.getLogger(__name__)


		class MessageThreadDAO(BaseDAO[MessageThread]):

[DRAFT]- Elizabeth/cord poc #605

[DRAFT]- Elizabeth/cord poc #605

Conversation

eschutho commented Sep 13, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 13, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Sep 26, 2023 •

edited

Loading