pierrejeambrun commented on code in PR #51264: URL: https://github.com/apache/airflow/pull/51264#discussion_r2159068035
########## airflow-core/src/airflow/ui/src/pages/Dashboard/FavoriteDags/FavoriteDags.tsx: ########## @@ -0,0 +1,96 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { Box, Flex, Heading, SimpleGrid, Text } from "@chakra-ui/react"; +import { useQueryClient } from "@tanstack/react-query"; +import { useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiStar } from "react-icons/fi"; +import { useLocation } from "react-router-dom"; + +import { useDagServiceGetDags } from "openapi/queries"; + +import { FavoriteDagCard } from "./FavoriteDagCard"; + +const MAX_VISIBLE = 5; + +export const FavoriteDags = () => { + const queryClient = useQueryClient(); + const location = useLocation(); + + const { t: translate } = useTranslation("dashboard"); + const { data: favorites } = useDagServiceGetDags({ isFavorite: true }); Review Comment: The UI needs to fetch dags with `useDagServiceGetDagsUi`. All other places where we need that are using the UI version. (and things are cached). (This is why you also need to propagate your backend change to the `ui` endpoint. `def get_dags` in the `ui/dags.py` file. ########## airflow-core/src/airflow/api_fastapi/core_api/routes/public/dags.py: ########## @@ -113,9 +116,12 @@ def get_dags( ], readable_dags_filter: ReadableDagsFilterDep, session: SessionDep, + user: GetUserDep, + is_favorite: QueryFavoriteFilter, ) -> DAGCollectionResponse: """Get all DAGs.""" - query = select(DagModel) + is_favorite.user_id = user.get_id() + query = is_favorite.to_orm(select(DagModel)) Review Comment: Why do we perform a manual call to the `to_orm` parameter? Passing the filter to `paginated_select` should do exactly that. ########## airflow-core/src/airflow/api_fastapi/core_api/routes/public/dags.py: ########## @@ -346,6 +352,64 @@ def patch_dags( ) +@dags_router.post( + "/{dag_id}/favorite", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), + dependencies=[Depends(requires_access_dag(method="GET")), Depends(action_logging())], +) +def favorite_dag( + dag_id: str, + session: SessionDep, + user: GetUserDep, +) -> DAGResponse: + """Mark the DAG as favorite.""" + dag = session.get(DagModel, dag_id) + if not dag: + raise HTTPException(status.HTTP_404_NOT_FOUND, detail=f"DAG with id '{dag_id}' not found") + + user_id = user.get_id() + session.execute(insert(DagFavorite).values(dag_id=dag_id, user_id=user_id)) + + return DAGResponse.model_validate(dag) Review Comment: lets try no content 204. ########## airflow-core/src/airflow/api_fastapi/core_api/routes/public/dags.py: ########## @@ -346,6 +358,55 @@ def patch_dags( ) +@dags_router.post( + "/{dag_id}/favorite", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), + dependencies=[Depends(requires_access_dag(method="POST")), Depends(action_logging())], +) +def favorite_dag( + dag_id: str, + session: SessionDep, + user: GetUserDep, +) -> DAGResponse: + """Mark the DAG as favorite.""" + dag = session.get(DagModel, dag_id) + if not dag: + raise HTTPException(status.HTTP_404_NOT_FOUND, detail=f"DAG with id '{dag_id}' not found") + + user_id = user.get_id() + session.execute(insert(DagFavorite).values(dag_id=dag_id, user_id=user_id)) + session.commit() + + return DAGResponse.model_validate(dag) + + +@dags_router.post( + "/{dag_id}/unfavorite", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), + dependencies=[Depends(requires_access_dag(method="POST")), Depends(action_logging())], +) +def unfavorite_dag( + dag_id: str, + session: SessionDep, + user: GetUserDep, +) -> DAGResponse: + """Unmark the DAG as favorite.""" + dag = session.get(DagModel, dag_id) + if not dag: + raise HTTPException(status.HTTP_404_NOT_FOUND, detail=f"DAG with id '{dag_id}' not found") + + user_id = user.get_id() + session.execute( + delete(DagFavorite).where( + DagFavorite.dag_id == dag_id, + DagFavorite.user_id == user_id, + ) + ) + session.commit() + + return DAGResponse.model_validate(dag) Review Comment: Yes maybe a 204 no content could be enough. ########## airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dags.py: ########## @@ -392,6 +420,90 @@ def test_patch_dags_should_response_403(self, unauthorized_test_client): assert response.status_code == 403 +class TestFavoriteDag(TestDagEndpoint): + """Unit tests for favoriting a DAG.""" + + @pytest.mark.parametrize( + "dag_id, expected_status_code, expected_exist_in_favorites", + [ + ("fake_dag_id", 404, None), + (DAG1_ID, 200, True), + ], + ) + def test_favorite_dag( + self, test_client, dag_id, expected_status_code, expected_exist_in_favorites, session + ): + response = test_client.post(f"/dags/{dag_id}/favorite") + assert response.status_code == expected_status_code + + if expected_status_code == 200: + result = session.execute( + select(DagFavorite).where(DagFavorite.dag_id == dag_id, DagFavorite.user_id == "test") + ).first() + assert result is not None if expected_exist_in_favorites else result is None + check_last_log(session, dag_id=dag_id, event="favorite_dag", logical_date=None) + + def test_favorite_dag_should_response_401(self, unauthenticated_test_client): + response = unauthenticated_test_client.post(f"/dags/{DAG1_ID}/favorite") + assert response.status_code == 401 + + def test_favorite_dag_should_response_403(self, unauthorized_test_client): + response = unauthorized_test_client.post(f"/dags/{DAG1_ID}/favorite") + assert response.status_code == 403 + + def test_favoriting_already_favorited_dag_returns_409(self, test_client): + response = test_client.post(f"/dags/{DAG1_ID}/favorite") + assert response.status_code == 200 + + response = test_client.post(f"/dags/{DAG1_ID}/favorite") + assert response.status_code == 409 + + +class TestUnfavoriteDag(TestDagEndpoint): + """Unit tests for unfavoriting a DAG.""" + + @pytest.mark.parametrize( + "dag_id, expected_status_code, expected_exist_in_favorites", + [ + ("fake_dag_id", 404, None), + (DAG1_ID, 200, False), + ], + ) + def test_unfavorite_dag( + self, test_client, dag_id, expected_status_code, expected_exist_in_favorites, session + ): + if dag_id != "fake_dag_id": + session.execute(insert(DagFavorite).values(dag_id=dag_id, user_id="test")) + session.commit() + + response = test_client.post(f"/dags/{dag_id}/unfavorite") + assert response.status_code == expected_status_code + + if expected_status_code == 200: + result = session.execute( + select(DagFavorite).where(DagFavorite.dag_id == dag_id, DagFavorite.user_id == "test") + ).first() + assert result is not None if expected_exist_in_favorites else result is None + check_last_log(session, dag_id=dag_id, event="unfavorite_dag", logical_date=None) + + def test_unfavorite_dag_should_response_401(self, unauthenticated_test_client): + response = unauthenticated_test_client.post(f"/dags/{DAG1_ID}/unfavorite") + assert response.status_code == 401 + + def test_unfavorite_dag_should_response_403(self, unauthorized_test_client): + response = unauthorized_test_client.post(f"/dags/{DAG1_ID}/unfavorite") + assert response.status_code == 403 + + def test_unfavoriting_dag_that_is_not_favorite_returns_409(self, test_client, session): + session.execute( + delete(DagFavorite).where(DagFavorite.dag_id == DAG1_ID, DagFavorite.user_id == "test") + ) + session.commit() Review Comment: To avoid having to do this and have test interfere with each other, you can take a look at the ` def _clear_db`. You'll need to add the removal of dag favorite to the `clear_db_dags`. This way you are sure all tests that `clear_db_dags` at the settup will start with a fresh clean favorite table. ########## airflow-core/src/airflow/ui/src/pages/Dashboard/FavoriteDags/FavoriteDags.tsx: ########## @@ -0,0 +1,96 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { Box, Flex, Heading, SimpleGrid, Text } from "@chakra-ui/react"; +import { useQueryClient } from "@tanstack/react-query"; +import { useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiStar } from "react-icons/fi"; +import { useLocation } from "react-router-dom"; + +import { useDagServiceGetDags } from "openapi/queries"; + +import { FavoriteDagCard } from "./FavoriteDagCard"; + +const MAX_VISIBLE = 5; Review Comment: You don't need to do that slicing in the front-end. Endpoint is paginated, just request the first 5. ########## airflow-core/src/airflow/api_fastapi/core_api/routes/public/dags.py: ########## @@ -113,9 +116,12 @@ def get_dags( ], readable_dags_filter: ReadableDagsFilterDep, session: SessionDep, + user: GetUserDep, + is_favorite: QueryFavoriteFilter, ) -> DAGCollectionResponse: """Get all DAGs.""" - query = select(DagModel) + is_favorite.user_id = user.get_id() Review Comment: You don't need to do that, `QueryFavoriteFilter` can access the dependency `GetUserDep`, just declare this in the function parameter that needs it. ########## airflow-core/src/airflow/ui/src/pages/Dashboard/FavoriteDags/FavoriteDags.tsx: ########## @@ -0,0 +1,96 @@ +/*! + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { Box, Flex, Heading, SimpleGrid, Text } from "@chakra-ui/react"; +import { useQueryClient } from "@tanstack/react-query"; +import { useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiStar } from "react-icons/fi"; +import { useLocation } from "react-router-dom"; + +import { useDagServiceGetDags } from "openapi/queries"; + +import { FavoriteDagCard } from "./FavoriteDagCard"; + +const MAX_VISIBLE = 5; + +export const FavoriteDags = () => { + const queryClient = useQueryClient(); + const location = useLocation(); + + const { t: translate } = useTranslation("dashboard"); + const { data: favorites } = useDagServiceGetDags({ isFavorite: true }); Review Comment: Also the endpoint is paginated, so it's not really a `showAll` but a show 'first page'. Maybe limit it to 10, and 'show' 10 first favorite dags. If you want to show all you will need a scroll infinite, you can take exemple in `useDagTagsInfinite`, but I recommend to do that in a follow up. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
