From dcf29086826919a093b31006963f44f5c5ba8cfe Mon Sep 17 00:00:00 2001 From: Paco Aranda Date: Fri, 8 Mar 2024 11:53:57 +0100 Subject: [PATCH 1/6] chore: load dataset with fields and questions --- src/argilla_server/apis/v1/handlers/datasets/questions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/argilla_server/apis/v1/handlers/datasets/questions.py b/src/argilla_server/apis/v1/handlers/datasets/questions.py index 0ebf8102..a0ab25ef 100644 --- a/src/argilla_server/apis/v1/handlers/datasets/questions.py +++ b/src/argilla_server/apis/v1/handlers/datasets/questions.py @@ -48,7 +48,7 @@ async def create_dataset_question( question_create: QuestionCreate, current_user: User = Security(auth.get_current_user), ): - dataset = await _get_dataset_or_raise(db, dataset_id, with_fields=True) + dataset = await _get_dataset_or_raise(db, dataset_id, with_fields=True, with_questions=True) await authorize(current_user, DatasetPolicyV1.create_question(dataset)) From 18491a7bd56ffaef7f9f3d6a415b7e41d277aa4f Mon Sep 17 00:00:00 2001 From: Paco Aranda Date: Fri, 8 Mar 2024 11:55:25 +0100 Subject: [PATCH 2/6] feat: Add validation to avoid several span questions using the same field --- src/argilla_server/contexts/questions.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/argilla_server/contexts/questions.py b/src/argilla_server/contexts/questions.py index e900531e..0cabfbbe 100644 --- a/src/argilla_server/contexts/questions.py +++ b/src/argilla_server/contexts/questions.py @@ -120,6 +120,10 @@ def _validate_span_question_settings_before_create( if field not in field_names: raise ValueError(f"'{field}' is not a valid field name.\nValid field names are {field_names!r}") + for question in dataset.questions: + if question.type == QuestionType.span and field == question.parsed_settings.field: + raise ValueError(f"'{field}' is already used by span question with id '{question.id}'") + async def create_question(db: AsyncSession, dataset: Dataset, question_create: QuestionCreate) -> Question: if dataset.is_ready: From 68e0101a0729206faae7f95ff8aa607331735e4a Mon Sep 17 00:00:00 2001 From: Paco Aranda Date: Fri, 8 Mar 2024 11:56:41 +0100 Subject: [PATCH 3/6] tests: Adding more tests --- .../datasets/test_create_dataset_question.py | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/tests/unit/api/v1/datasets/test_create_dataset_question.py b/tests/unit/api/v1/datasets/test_create_dataset_question.py index 394dd155..d9b397de 100644 --- a/tests/unit/api/v1/datasets/test_create_dataset_question.py +++ b/tests/unit/api/v1/datasets/test_create_dataset_question.py @@ -22,7 +22,7 @@ from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession -from tests.factories import DatasetFactory, TextFieldFactory +from tests.factories import DatasetFactory, SpanQuestionFactory, TextFieldFactory @pytest.mark.asyncio @@ -109,3 +109,34 @@ async def test_create_dataset_span_question_with_non_existent_field( } assert (await db.execute(select(func.count(Question.id)))).scalar() == 0 + + async def test_create_dataset_question_with_already_span_question_using_the_same_field( + self, async_client: AsyncClient, db: AsyncSession, owner_auth_header: dict + ): + dataset = await DatasetFactory.create() + question = await SpanQuestionFactory(dataset=dataset) + + question_field = question.settings["field"] + await TextFieldFactory.create(name=question_field, dataset=dataset) + + response = await async_client.post( + self.url(dataset.id), + headers=owner_auth_header, + json={ + "name": "new-question", + "title": "Title", + "settings": { + "type": QuestionType.span, + "field": question_field, + "options": [ + {"value": "label-c", "text": "Label C", "description": "Label C description"}, + {"value": "label-d", "text": "Label D", "description": "Label D description"}, + ], + }, + }, + ) + + assert response.status_code == 422 + assert response.json() == { + "detail": f"'field-a' is already used by span question with id '{question.id}'" + } From cfb72e530169423a7111f254046f184eb73eb53d Mon Sep 17 00:00:00 2001 From: Paco Aranda Date: Fri, 8 Mar 2024 11:58:52 +0100 Subject: [PATCH 4/6] chore: Add TODO --- src/argilla_server/apis/v1/handlers/datasets/questions.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/argilla_server/apis/v1/handlers/datasets/questions.py b/src/argilla_server/apis/v1/handlers/datasets/questions.py index a0ab25ef..cf9e22f7 100644 --- a/src/argilla_server/apis/v1/handlers/datasets/questions.py +++ b/src/argilla_server/apis/v1/handlers/datasets/questions.py @@ -48,6 +48,8 @@ async def create_dataset_question( question_create: QuestionCreate, current_user: User = Security(auth.get_current_user), ): + # TODO: Review this flow since we're putting logic here that will be used internally by the context + # Fields and questions are required to apply validations. dataset = await _get_dataset_or_raise(db, dataset_id, with_fields=True, with_questions=True) await authorize(current_user, DatasetPolicyV1.create_question(dataset)) From dfe0c34f92401379d0e8eca44c30a1e88b9d8674 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 8 Mar 2024 10:59:13 +0000 Subject: [PATCH 5/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/unit/api/v1/datasets/test_create_dataset_question.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/api/v1/datasets/test_create_dataset_question.py b/tests/unit/api/v1/datasets/test_create_dataset_question.py index d9b397de..efa7457e 100644 --- a/tests/unit/api/v1/datasets/test_create_dataset_question.py +++ b/tests/unit/api/v1/datasets/test_create_dataset_question.py @@ -137,6 +137,4 @@ async def test_create_dataset_question_with_already_span_question_using_the_same ) assert response.status_code == 422 - assert response.json() == { - "detail": f"'field-a' is already used by span question with id '{question.id}'" - } + assert response.json() == {"detail": f"'field-a' is already used by span question with id '{question.id}'"} From b1ce3bf401ba1b219aa629e2e2741cfab18f6d0b Mon Sep 17 00:00:00 2001 From: Francisco Aranda Date: Fri, 8 Mar 2024 14:43:47 +0100 Subject: [PATCH 6/6] Update tests/unit/api/v1/datasets/test_create_dataset_question.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: José Francisco Calvo --- tests/unit/api/v1/datasets/test_create_dataset_question.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/api/v1/datasets/test_create_dataset_question.py b/tests/unit/api/v1/datasets/test_create_dataset_question.py index efa7457e..4fc40b90 100644 --- a/tests/unit/api/v1/datasets/test_create_dataset_question.py +++ b/tests/unit/api/v1/datasets/test_create_dataset_question.py @@ -110,7 +110,7 @@ async def test_create_dataset_span_question_with_non_existent_field( assert (await db.execute(select(func.count(Question.id)))).scalar() == 0 - async def test_create_dataset_question_with_already_span_question_using_the_same_field( + async def test_create_dataset_question_with_other_span_question_using_the_same_field( self, async_client: AsyncClient, db: AsyncSession, owner_auth_header: dict ): dataset = await DatasetFactory.create()