feat(Tags): Allow users to favorite Tags on CRUD Listview page #24701

hughhhh · 2023-07-14T20:05:57Z

SUMMARY

Update CRUD Tag Page to have favorites stars, and filter dropdowns from @kasiazjc designs. The main change is now allowing users to favorite tags

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

codecov · 2023-07-14T20:15:52Z

Codecov Report

Merging #24701 (ef55538) into master (165afee) will increase coverage by 0.14%.
Report is 5 commits behind head on master.
The diff coverage is 79.72%.

❗ Current head ef55538 differs from pull request most recent head c1a6082. Consider uploading reports for the commit c1a6082 to get more accurate results

@@            Coverage Diff             @@
##           master   #24701      +/-   ##
==========================================
+ Coverage   68.84%   68.98%   +0.14%     
==========================================
  Files        1902     1903       +1     
  Lines       73997    74086      +89     
  Branches     8195     8194       -1     
==========================================
+ Hits        50944    51110     +166     
+ Misses      20934    20860      -74     
+ Partials     2119     2116       -3

Flag	Coverage Δ
hive	`54.14% <46.83%> (?)`
javascript	`55.81% <73.91%> (+0.01%)`	⬆️
mysql	`79.21% <84.81%> (+0.01%)`	⬆️
postgres	`79.31% <84.81%> (+0.01%)`	⬆️
presto	`54.04% <46.83%> (-0.02%)`	⬇️
python	`83.36% <84.81%> (+0.24%)`	⬆️
sqlite	`77.88% <84.81%> (?)`
unit	`54.97% <64.55%> (+0.01%)`	⬆️

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

Files Changed	Coverage Δ
superset-frontend/src/SqlLab/App.jsx	`0.00% <ø> (ø)`
...tersConfigModal/Footer/CancelConfirmationAlert.tsx	`100.00% <ø> (ø)`
superset-frontend/src/hooks/apiResources/tables.ts	`73.80% <ø> (ø)`
superset-frontend/src/pages/Tags/index.tsx	`0.00% <0.00%> (ø)`
superset-frontend/src/views/CRUD/hooks.ts	`52.38% <ø> (ø)`
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx	`71.09% <50.00%> (+0.13%)`	⬆️
...t-frontend/src/components/AsyncAceEditor/index.tsx	`91.66% <63.63%> (+0.55%)`	⬆️
superset/tags/api.py	`75.16% <77.27%> (+0.85%)`	⬆️
.../SqlLab/components/AceEditorWrapper/useKeywords.ts	`80.00% <80.00%> (ø)`
superset/daos/tag.py	`95.41% <93.10%> (-0.89%)`	⬇️
... and 4 more

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zephyring · 2023-07-14T20:49:04Z

superset/daos/tag.py

+
+        if tag and user:
+            tag.users_favorited.append(user)
+            db.session.add(tag)


should we use session.merge as this tag object is returned object from the DAO?

whats the difference between just doing add/commit?

see https://docs.sqlalchemy.org/en/20/orm/session_state_management.html
and https://stackoverflow.com/questions/1849567/can-sqlalchemys-session-merge-update-its-result-with-newer-data-from-the-data

I believe since you get tag from the TagDAO the tag object is already in the session.
Calling add here is redundant.
But I am not sure about that g.user if that's returning a user object in the same session or if it's in a different session.
If it's in different sqla session, you will want to call merge() here so that it merges the user object to current session by matching the user.id.

Regardless, I think merge() is safer.

Agree. We should pass the user as a param as opposed to using the global

zephyring · 2023-07-14T20:50:36Z

superset/daos/tag.py

@@ -257,3 +266,90 @@ def get_tagged_objects_for_tags(
                for obj in saved_queries
            )
        return results
+
+    @staticmethod
+    def user_favorite_tag(tag_id: int) -> None:


naming: maybe favor_tag_by_id_for_current_user

or you can pass in user_id as param so it will be like def favor_tag_by_id(tag_id: int, user_id: int) -> None

is there any authorization that you need to check or any user can favor any tag?

right now any user can favorite a tag and matches the current functionality for other resources

zephyring · 2023-07-14T20:55:51Z

superset/daos/tag.py

+        db.session.commit()
+
+    @staticmethod
+    def favorited_ids(tags: list[Tag]) -> list[int]:


naming: favorited_tag_ids

I think it's better to just pass in tag IDs instead of the tag objects

i want to follow the same convention i the other DAOs:
https://github.com/preset-io/superset/blob/84c183e0e6005738b0dad60a30824b17a087a752/superset/daos/chart.py#L73

superset/tags/api.py

+              $ref: '#/components/responses/500'
+        """
+        requested_ids = kwargs["rison"]
+        tags = TagDAO.find_by_ids(requested_ids)


zephyring · 2023-07-14T21:00:27Z

superset/daos/tag.py

+        Returns:
+            None.
+        """
+        tag = TagDAO.find_by_id(tag_id)


we should check tag is None here and throw.
Then in the controller you catch the TagNotFoundException and return 404 http code

superset/daos/tag.py

john-bodley · 2023-07-17T16:54:18Z

@hughhhh would you mind including screenshots (or the designs) to help provide more context.

john-bodley · 2023-07-17T16:56:40Z

superset/daos/tag.py

+
+        # Remove the tag from the user's favorites
+        if tag and user:
+            tag.users_favorited.remove(user)


What's the TL;DR between tags and favoriting? It seems like we're doing double bookkeeping here which is likely suboptimal.

tags will become the defacto for managing favorites moving forward, i have a ticket to manage migration and handle the cleanup.

This table is needed becuase we can leverage tags to favorite tags with the circuliar dependency

john-bodley · 2023-07-18T20:59:07Z

superset/daos/tag.py

+        Returns:
+            None.
+        """
+        # Find the tag


This comment seems superfluous. Well named methods FTW!

john-bodley · 2023-07-18T20:59:27Z

superset/daos/tag.py

+        tag = TagDAO.find_by_id(tag_id)
+        user = g.user
+
+        # Remove the tag from the user's favorites


See previous comment.

zephyring · 2023-07-25T18:25:52Z

superset/tags/api.py

+            500:
+              $ref: '#/components/responses/500'
+        """
+        TagDAO.favorite_tag_by_id_for_current_user(pk)


do you need to handle UserNotFound and TagNotFound exception here?

zephyring · 2023-07-25T18:26:16Z

superset/tags/api.py

+        if not TagDAO.find_by_id(pk):
+            return self.response_404()
+
+        TagDAO.remove_user_favorite_tag(pk)


do you need to handle UserNotFound and TagNotFound exception here? You can remove line 516 then

zephyring · 2023-07-25T18:39:30Z

tests/integration_tests/tags/api_tests.py

@@ -372,3 +372,47 @@ def test_delete_tags(self):
        # check that tags are all gone
        tags = db.session.query(Tag).filter(Tag.name.in_(example_tag_names))
        self.assertEqual(tags.count(), 0)
+
+    @pytest.mark.usefixtures("create_tags")
+    def test_add_delete_favorite_tag(self):


add some negative tests for bad tag id

tests/unit_tests/dao/tag_test.py

+    mock_session.commit.assert_called_once()
+
+
+def test_remove_user_favorite_tag(mocker):


…xgh-update-tags-page

zephyring · 2023-07-26T01:56:41Z

tests/integration_tests/tags/api_tests.py

+    @pytest.mark.usefixtures("create_tags")
+    def test_add_tag_not_found(self):
+        self.login(username="admin")
+        user_id = self.get_user(username="admin").get_id()


user_id is not used

…xgh-update-tags-page

Antonio-RiveroMartnez

lgtm

hughhhh requested a review from a team as a code owner July 14, 2023 20:05

pull-request-size bot added the size/L label Jul 14, 2023

hughhhh changed the title ~~feat: allow users to favorite Tags~~ feat(Tags): Allow users to favorite Tags on CRUD Listview page Jul 14, 2023

create dao + unit test

4d18262

hughhhh force-pushed the hxgh-update-tags-page branch from 05e3e8a to 4d18262 Compare July 14, 2023 20:25

zephyring reviewed Jul 14, 2023

View reviewed changes

superset/tags/api.py Outdated

$ref: '#/components/responses/500'

"""

requested_ids = kwargs["rison"]

tags = TagDAO.find_by_ids(requested_ids)

This comment was marked as outdated.

Sign in to view

zephyring reviewed Jul 14, 2023

View reviewed changes

superset/daos/tag.py Show resolved Hide resolved

john-bodley reviewed Jul 17, 2023

View reviewed changes

john-bodley reviewed Jul 18, 2023

View reviewed changes

address concerns

85f90b5

hughhhh force-pushed the hxgh-update-tags-page branch 2 times, most recently from b7a5ae5 to 5c414ed Compare July 19, 2023 17:57

pull-request-size bot added size/XL and removed size/L labels Jul 19, 2023

hughhhh force-pushed the hxgh-update-tags-page branch from 5c414ed to 0943192 Compare July 19, 2023 17:59

pull-request-size bot added size/L and removed size/XL labels Jul 19, 2023

hughhhh force-pushed the hxgh-update-tags-page branch from 0943192 to 6948f13 Compare July 19, 2023 18:51

pull-request-size bot added size/XL and removed size/L labels Jul 19, 2023

hughhhh force-pushed the hxgh-update-tags-page branch from 6948f13 to 2a29be1 Compare July 19, 2023 18:53

pull-request-size bot added size/L and removed size/XL labels Jul 19, 2023

fix linting

5437846

hughhhh force-pushed the hxgh-update-tags-page branch from 3d3eaa1 to bc20c0b Compare July 25, 2023 18:23

zephyring reviewed Jul 25, 2023

View reviewed changes

tests/unit_tests/dao/tag_test.py

mock_session.commit.assert_called_once()

def test_remove_user_favorite_tag(mocker):

This comment was marked as outdated.

Sign in to view

change migration order

8c5772b

hughhhh force-pushed the hxgh-update-tags-page branch from bc20c0b to 8c5772b Compare July 25, 2023 18:57

hughhhh added 3 commits July 25, 2023 19:12

adding test

cfeb4af

Merge branch 'master' of https://github.com/preset-io/superset into h…

0b348f4

…xgh-update-tags-page

fix migration again

e1aa033

pull-request-size bot added size/XL and removed size/L labels Jul 26, 2023

zephyring reviewed Jul 26, 2023

View reviewed changes

updated test

903cdaa

hughhhh force-pushed the hxgh-update-tags-page branch from 57a00a3 to 903cdaa Compare July 26, 2023 01:58

hughhhh added 2 commits July 26, 2023 16:56

Merge branch 'master' of https://github.com/preset-io/superset into h…

894066e

…xgh-update-tags-page

address more concerns

5878148

hughhhh force-pushed the hxgh-update-tags-page branch 5 times, most recently from 88fd3a4 to 4e01884 Compare July 26, 2023 21:31

more test

c1a6082

hughhhh force-pushed the hxgh-update-tags-page branch from 4e01884 to c1a6082 Compare July 26, 2023 21:35

zephyring approved these changes Jul 26, 2023

View reviewed changes

Antonio-RiveroMartnez approved these changes Jul 27, 2023

View reviewed changes

hughhhh merged commit 3b46511 into apache:master Jul 27, 2023
29 checks passed

hughhhh deleted the hxgh-update-tags-page branch July 27, 2023 17:17

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Tags): Allow users to favorite Tags on CRUD Listview page #24701

feat(Tags): Allow users to favorite Tags on CRUD Listview page #24701

hughhhh commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

zephyring Jul 14, 2023

hughhhh Jul 18, 2023

zephyring Jul 19, 2023

craig-rueda Jul 19, 2023

zephyring Jul 14, 2023

zephyring Jul 14, 2023

zephyring Jul 14, 2023

hughhhh Jul 18, 2023

zephyring Jul 14, 2023

zephyring Jul 14, 2023

hughhhh Jul 18, 2023

This comment was marked as outdated.

zephyring Jul 14, 2023

john-bodley commented Jul 17, 2023

john-bodley Jul 17, 2023

hughhhh Jul 18, 2023

john-bodley Jul 18, 2023

john-bodley Jul 18, 2023

zephyring Jul 25, 2023

zephyring Jul 25, 2023

zephyring Jul 25, 2023

This comment was marked as outdated.

zephyring Jul 26, 2023

Antonio-RiveroMartnez left a comment

		mock_session.commit.assert_called_once()


		def test_remove_user_favorite_tag(mocker):

feat(Tags): Allow users to favorite Tags on CRUD Listview page #24701

feat(Tags): Allow users to favorite Tags on CRUD Listview page #24701

Conversation

hughhhh commented Jul 14, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 14, 2023 • edited Loading

Codecov Report

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

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

This comment was marked as outdated.

Choose a reason for hiding this comment

john-bodley commented Jul 17, 2023

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

This comment was marked as outdated.

Choose a reason for hiding this comment

Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

hughhhh commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading