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.

Sign up for GitHub

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

Jump to bottom

Add Teams model #552

Merged
merged 3 commits into from Jul 7, 2021
Merged

Add Teams model #552

merged 3 commits into from Jul 7, 2021

Conversation

Rashmi-K-A
Copy link
Contributor

@Rashmi-K-A Rashmi-K-A commented Jun 22, 2021 •
edited

This PR is in reference to issue #548 for creating a Groups Teams model to handle organizational hierarchies. This PR includes changes to the following files:

  • models.py - add Team model
  • db.py - add internal methods add_team, find_team and delete_team
  • api.py - add methods add_team, delete_team

#541 has more information on the discussion around how to handle organizational hierarchies.

sduenas
sduenas requested changes Jun 23, 2021
View reviewed changes
Copy link
Member

@sduenas sduenas left a comment •
edited

Choose a reason for hiding this comment

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

In general it looks good but some changes are needed. Please also check and modify the style in the tests and with the docstrings.
Additionally, you don't need that merge commit.

sortinghat/core/migrations/0009_auto_20210622_1033.py Outdated Show resolved Hide resolved
sortinghat/core/models.py Outdated Show resolved Hide resolved
sortinghat/core/models.py Outdated Show resolved Hide resolved
tests/test_model.py Outdated Show resolved Hide resolved
sortinghat/core/db.py Outdated Show resolved Hide resolved
tests/test_db.py Outdated Show resolved Hide resolved
sortinghat/core/api.py Outdated Show resolved Hide resolved
sortinghat/core/api.py Outdated Show resolved Hide resolved
sortinghat/core/api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
sduenas
sduenas requested changes Jun 29, 2021
View reviewed changes
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.

In overall it looks ok but I don't like much the way you're solving the integrity problem with the same name team without organization.
You also need to remove that merge commit that is there.

sortinghat/core/db.py Outdated Show resolved Hide resolved
sortinghat/core/db.py Show resolved Hide resolved
sortinghat/core/models.py Outdated Show resolved Hide resolved
tests/test_api.py Show resolved Hide resolved
tests/test_db.py Show resolved Hide resolved
@Rashmi-K-A
Copy link
Contributor Author

Rashmi-K-A commented Jun 29, 2021 •
edited

You also need to remove that merge commit that is there.

Yes, I have a call with Venu today to fix that. I thought its better to let him show me how to fix that the first time!

@Rashmi-K-A Rashmi-K-A requested a review from sduenas July 5, 2021 12:09
@Rashmi-K-A Rashmi-K-A changed the title Add Groups model Add Teams model Jul 5, 2021
@Rashmi-K-A
Copy link
Contributor Author

@sduenas I have fixed the commits (thanks to @vchrombie!). We were wondering about the changelog entry. Should that be added as part of this PR?

@sduenas
Copy link
Member

sduenas commented Jul 6, 2021

@sduenas I have fixed the commits (thanks to @vchrombie!). We were wondering about the changelog entry. Should that be added as part of this PR?

Maybe we can skip that part until the implementation with graphql + ui is done.

@sduenas
Copy link
Member

sduenas commented Jul 6, 2021

@sduenas I have fixed the commits (thanks to @vchrombie!). We were wondering about the changelog entry. Should that be added as part of this PR?

@Rashmi-K-A would you mind to change the style of the commits again? The body of the message should be limited to 70 characters or so. Now they are very long and only good for large screens.

@vchrombie
Copy link
Member

@Rashmi-K-A
Good work with the re-writing the commits, but I think we have some more things to do.
You can truncate the commit description into multiple lines.

@vchrombie
Copy link
Member

@Rashmi-K-A would you mind to change the style of the commits again? The body of the message should be limited to 70 characters or so. Now they are very long and only good for large screens.

Jinx! 😄 🥂

@vchrombie
Copy link
Member

@Rashmi-K-A let me know if you need help with anything.

@Rashmi-K-A
[model] Add Teams model
e4ff708 
Adds a model called `Team` that can be used to represent
organizational units. The Team model stores the `name` of
the team and links to the Organization object it belongs to.

Signed-off-by: Rashmi-K-A <k.a.rashmi04@gmail.com>
@Rashmi-K-A
[db] Add internal methods for handling teams
6bab7d3 
Adds three functions to handle Team objects:
 `add_team`, `find_team`, `delete_team`.

The function add_team() adds a new Team object to the registry.
The Team can optionally belong to an Organization. It can also
optionally be created under an existing Team object as a subteam
to represent an organizational hierarchy.

The function find_team() finds a Team by its name in the registry.

The function delete_team() deletes a Team object and all related
objects (i.e all subteams) from the database.

Signed-off-by: Rashmi-K-A <k.a.rashmi04@gmail.com>
@Rashmi-K-A
[api] Add api methods for teams
c709874 
The function add_team() allows users to create a new Team object.

The function delete_team() allows users to delete existing Team
objects and all of their subteams.

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

Thanks @vchrombie @sduenas !! I have fixed it now 💯 Let me know if I need to change anything else!

sduenas
sduenas approved these changes Jul 7, 2021
View reviewed changes
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 ad9c290 into chaoss:muggle Jul 7, 2021
@sduenas sduenas mentioned this pull request Jul 7, 2021
Closed
3 tasks
@sduenas
Copy link
Member

sduenas commented Jul 7, 2021

Merged. Thanks for your contribution!

@Rashmi-K-A Rashmi-K-A deleted the add_groups branch July 7, 2021 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sduenas sduenas sduenas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Rashmi-K-A @sduenas @vchrombie