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

✨(teams) add Team dependencies #560

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

✨(teams) add Team dependencies #560

wants to merge 5 commits into from

Conversation

qbey
Copy link
Collaborator

@qbey qbey commented Nov 26, 2024

Purpose

We want to allow Teams to be structured in trees to represent actual teams in a company.

Proposal

Add the "tree" to the Team model and update the API endpoints to list parent teams along with the "direct" teams the user has access to.

This first implementation only allows to manually create those team trees.
The API returns flat teams because we did not define the frontend behavior yet, this will be done later when we really know what we want.

The Treebeard has been pimped for PSQL use, and make the different values explicit.

  • add django-treebeard dependency
  • turn the Team model into an MP node
  • update APIs to list parent teams if the user belongs to the same organization only

@qbey qbey force-pushed the qbey/teams-add-tree branch from 34da950 to 312bb54 Compare November 27, 2024 09:38
@qbey qbey force-pushed the qbey/teams-add-tree branch from 312bb54 to 0d2b578 Compare December 5, 2024 14:24
@qbey qbey self-assigned this Dec 6, 2024
@qbey qbey force-pushed the qbey/teams-add-tree branch 7 times, most recently from ed99015 to 9ad2678 Compare December 18, 2024 10:43
@qbey qbey added the backend label Dec 18, 2024
@qbey qbey changed the title 🚧(teams) add Team dependencies ✨(teams) add Team dependencies Dec 18, 2024
@qbey qbey force-pushed the qbey/teams-add-tree branch from 9ad2678 to 94ab00e Compare December 18, 2024 10:49
@qbey qbey marked this pull request as ready for review December 18, 2024 10:49
@qbey qbey requested review from sampaccoud and sdemagny December 18, 2024 10:49
@qbey qbey force-pushed the qbey/teams-add-tree branch 3 times, most recently from 500ec8f to 7e6037b Compare December 18, 2024 17:34
@qbey qbey force-pushed the qbey/teams-add-tree branch 4 times, most recently from e7202a1 to 9b32890 Compare January 6, 2025 13:05
@qbey qbey force-pushed the qbey/teams-add-tree branch 5 times, most recently from a2e7d74 to dc848dd Compare January 8, 2025 13:28
@qbey qbey requested a review from sampaccoud January 13, 2025 14:43
# This might be changed later if we never do any operation on the parent object
# before creating the child.
# Beware the N+1 here.
return self.get(pk=parent_id).add_child(**kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not expect the parent object directly? It seems to me what we expect when playing with objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't use the object directly to be safer (even if sub-optimized) because as stated in django-treebeard documentation, it's important to have nodes up to date when doing such operations.

Do you think I should update the comment?

alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
# Django treebeard does not allow max_length = None...
steplen = 5
path = models.CharField(max_length=5 * 80, unique=True, db_collation="C")
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if this collation impacts the use of "startswith" 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using the C collation can improve database performance for certain operations, like sorting and comparison, because it uses simple byte-wise comparison rather than more complex locale-aware rules. However, this comes at the cost of losing locale-specific behaviors, such as case insensitivity and proper sorting for different languages.

So I guess it's only better :) Actually it still works properly in the specific case of "startswith" but I can't tell for the performance improvement or not.

pytestmark = pytest.mark.django_db


@override_settings(DEBUG=True)
@mock.patch.dict(defaults.NB_OBJECTS, TEST_NB_OBJECTS)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not mock? It allows to run it with less objects in tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but the tests were not failing with the amount of objects in this tests, yet the command was failing. So I guess it was better to test the command as it is.

name = fake.domain_name()

if name in excluded:
raise RuntimeError("Could not find a domain name that is not already created")
Copy link
Contributor

Choose a reason for hiding this comment

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

or add a iteration number aftter the domain? 🤷‍♂️


def test_models_teams_delete_team():
"""
Delete a parent team also deletes children.
Copy link
Contributor

Choose a reason for hiding this comment

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

in practice this will force you to introduce soft delete soon enough! 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably ^^ but for now this feature is hidden from end users, so I keep it for later when we need it.

qbey added 5 commits January 14, 2025 16:28
This provides the technical way to create Team trees.
The implementation is quite naive.
Also return the parent teams in the user's team endpoints.

This is a first implementation which returns a flat list
of teams (not a tree). This is not really helpful, but
it allows to create hierarchical teams manually (via
admin) if an organization needs it.
Also return the parent teams in the user's team endpoints.
This will allow the frontend to represent teams as a
tree if needed.
@qbey qbey force-pushed the qbey/teams-add-tree branch from dc848dd to 26b1880 Compare January 14, 2025 15:31
@qbey qbey requested a review from sampaccoud January 14, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants