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

[schema] Add GraphQL layer for teams #562

Merged
merged 1 commit into from Jul 19, 2021

Conversation

Rashmi-K-A
Copy link
Contributor

@Rashmi-K-A Rashmi-K-A commented Jul 12, 2021

This PR includes the changes for extending the GraphQL layer to manipulate Team objects. It adds a query for searching through Team objects and two mutations - AddTeam and DeleteTeam for creating and deleting Team objects respectively.

This PR closes #558.

@Rashmi-K-A Rashmi-K-A force-pushed the add_groups_graphql branch 2 times, most recently from 9604611 to bb51cae Compare July 12, 2021 15:09
Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me. Good job. Before we merging it, can you rebase it with muggle branch (I merged your other PR before).

I also have another question and I don't know if we can fix it in this PR. Probably we can do it in a different one. The thing is, when we run a query to ask for Teams, the fields related to the django tree package are visible. I think those fields shouldn't be exposed with the api cause they are details of the implementation but maybe I'm wrong. Also, if I understand correctly, it's not possible to ask for the children of a team. That's something that we should do because it's part of the idea of GraphQL.

@Rashmi-K-A
Copy link
Contributor Author

The PR looks good to me. Good job. Before we merging it, can you rebase it with muggle branch (I merged your other PR before).

I also have another question and I don't know if we can fix it in this PR. Probably we can do it in a different one. The thing is, when we run a query to ask for Teams, the fields related to the django tree package are visible. I think those fields shouldn't be exposed with the api cause they are details of the implementation but maybe I'm wrong. Also, if I understand correctly, it's not possible to ask for the children of a team. That's something that we should do because it's part of the idea of GraphQL.

@sduenas:

About the implementation details - I think am a bit confused here, it doesnt return any of the fields like path and depth and num children in the graphQL response. Did you mean something else?

About getting the children - I was thinking of this, it is just a different function call getChildren instead of getDescendants, maybe we can add a bool filter that can toggle based on whether users want just the children or all descendants. Or if you think we dont have a need for users to see all descendants, we can just use getChildren instead.

This commit adds the queries and mutations for handling Team objects in schema.py.

The `AddTeam` mutation allows users to a new team to the registry.
The `DeleteTeam` mutation allows users to delete existing teams from the registry.

The `teams` query has four filters that can be used to search through teams in
the registry. The filters are: name, term, organization and parent.

Signed-off-by: Rashmi-K-A <k.a.rashmi04@gmail.com>
@sduenas
Copy link
Member

sduenas commented Jul 16, 2021

The PR looks good to me. Good job. Before we merging it, can you rebase it with muggle branch (I merged your other PR before).
I also have another question and I don't know if we can fix it in this PR. Probably we can do it in a different one. The thing is, when we run a query to ask for Teams, the fields related to the django tree package are visible. I think those fields shouldn't be exposed with the api cause they are details of the implementation but maybe I'm wrong. Also, if I understand correctly, it's not possible to ask for the children of a team. That's something that we should do because it's part of the idea of GraphQL.

@sduenas:

About the implementation details - I think am a bit confused here, it doesnt return any of the fields like path and depth and num children in the graphQL response. Did you mean something else?

It does. For example, if you run this query:

query teams {
  teams {
    entities {
      name
      path
      numchild
    }
  }
}

I get the next results for the two teams that I created:

{
  "data": {
    "teams": {
      "entities": [
        {
          "name": "test",
          "path": "0001",
          "numchild": 0
        },
        {
          "name": "test2",
          "path": "0002",
          "numchild": 0
        }
      ]
    }
  }
}

This is because we're exposing these fields from the internal model. I'm not sure if it's possible to avoid it.

About getting the children - I was thinking of this, it is just a different function call getChildren instead of getDescendants, maybe we can add a bool filter that can toggle based on whether users want just the children or all descendants. Or if you think we dont have a need for users to see all descendants, we can just use getChildren instead.

The descendants should be resolved within the query. I don't think we should add any filter to that. It should be something like:

query teams {
  teams {
    entities {
      name
      subteams {
         name
         subteams {
           name
           ...
      }
    }
  }

It's not mandatory to have it now but I think it's something we need because it's kind of the purpose of having graphql and a REST API, for example.

@Rashmi-K-A
Copy link
Contributor Author

Rashmi-K-A commented Jul 18, 2021

@sduenas:
About the implementation details - I think am a bit confused here, it doesnt return any of the fields like path and depth and num children in the graphQL response. Did you mean something else?

It does. For example, if you run this query:

query teams {
  teams {
    entities {
      name
      path
      numchild
    }
  }
}

I get the next results for the two teams that I created:

{
  "data": {
    "teams": {
      ...
  }
}

This is because we're exposing these fields from the internal model. I'm not sure if it's possible to avoid it.

@sduenas Oh! I did not notice that. Can we not use exclude=("path", "depth",.....) on the TeamType in schema.py to exclude certain fields?

@Rashmi-K-A
Copy link
Contributor Author

About getting the children - I was thinking of this, it is just a different function call getChildren instead of getDescendants, maybe we can add a bool filter that can toggle based on whether users want just the children or all descendants. Or if you think we dont have a need for users to see all descendants, we can just use getChildren instead.

The descendants should be resolved within the query. I don't think we should add any filter to that. It should be something like:

query teams {
  teams {
    entities {
      name
      subteams {
         name
         subteams {
           name
           ...
      }
    }
  }

It's not mandatory to have it now but I think it's something we need because it's kind of the purpose of having graphql and a REST API, for example.

Sure, I think we can work out a way to do that. I ll make note of it for now and we can circle back to it when the UI is done.

Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sduenas sduenas merged commit cfb6218 into chaoss:muggle Jul 19, 2021
@sduenas
Copy link
Member

sduenas commented Jul 19, 2021

I merged the PR but I'm going to open two issues to try to resolve the open questions of this PR.

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.

None yet

2 participants