Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update project tags type to what is expected/returned by API #150

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

dsciacca
Copy link
Contributor

@dsciacca dsciacca commented Jun 5, 2023

Description

This PR updates the type hints of the tags property of the Project class to align with what is expected and returned by the Doccano API. This addresses issues around creating, updating, and retrieving Doccano projects that have tags associated. Now that the issue mentioned in #149 is addressed I've found that the DoccanoClient.update_project() method fails to actually update tags when that kwarg is passed. Will open a separate issue for this since you can't actually reproduce the failed update of tags unless this PRs change is part of your local doccano-client

Fixes #149 (Unable to retrieve projects with tags)

Reproduction of issue before fix:
find_project_by_id():
Screenshot 2023-06-05 at 9 19 33 AM
list_projects():
Screenshot 2023-06-05 at 9 19 03 AM
create_project() w/ tags as specified in docs:
Screenshot 2023-06-05 at 9 17 59 AM
create_project() w/ tags as List[Dict[str, str]]:
Screenshot 2023-06-05 at 9 17 11 AM
update_project() w/ tags as specified in docs:
Screenshot 2023-06-05 at 9 16 03 AM
update_project() w/ tags as List[Dict[str, str]]:
Screenshot 2023-06-05 at 9 16 22 AM

Demonstration of successful fix using client created from this branch:
find_project_by_id():
Screenshot 2023-06-05 at 9 22 12 AM
list_projects() (truncated but demonstrates no failure):
Screenshot 2023-06-05 at 9 23 16 AM
create_project():
Screenshot 2023-06-05 at 9 25 07 AM
update_project() here you'll notice tags aren't actually updated but we can tell the call succeeded as the description was updated, this seems to be a separate bug only reproducible once this PRs fix is in place:
Screenshot 2023-06-05 at 9 33 47 AM

NOTE: This work was done as part of my employment at GoGuardian, Inc.

@dsciacca dsciacca requested a review from Hironsan as a code owner June 5, 2023 20:29
doccano_client/client.py Outdated Show resolved Hide resolved
@Hironsan
Copy link
Member

Hironsan commented Jun 6, 2023

Can this problem be fixed by adding and using the following two methods in ProjectRepository? I think this does not change the signature.

def to_domain(response: dict[str, Any]) -> Project:
    """Convert a response to a domain object

    Args:
        response (dict[str, Any]): The response to convert

    Returns:
        Project: The converted project
    """
    response["tags"] = [tag["text"] for tag in response["tags"]]
    return Project.parse_obj(response)


def to_persistent(project: Project) -> dict[str, Any]:
    """Convert a domain object to a persistent object

    Args:
        project (Project): The project to convert

    Returns:
        dict[str, Any]: The converted project
    """
    payload = project.dict()
    payload["tags"] = [{"text": tag} for tag in payload["tags"]]
    return payload

...
    def find_by_id(self, project_id: int) -> Project:
        response = self._client.get(f"projects/{project_id}")
        return to_domain(response.json())

    def create(self, project: Project) -> Project:
        payload = to_persistent(project)
        response = self._client.post("projects", json=payload)
        return to_domain(response.json())

@dsciacca
Copy link
Contributor Author

dsciacca commented Jun 6, 2023

@Hironsan good call, this also works and I agree is a better solution. I've reverted my original change and implemented your suggestion w/ some minor changes - i.e. make functions private class methods, remove "id" from the creation dict, if present, and align w/ expectations of unit tests. Please take a look and let me know your thoughts. Thanks!

@Hironsan Hironsan merged commit 12065e3 into doccano:master Jun 13, 2023
2 checks passed
@Hironsan
Copy link
Member

Thanks! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to retrieve projects with tags
2 participants