Skip to content

53 backend core core model group entity implementation 2 - #180

Merged
thomasdejaeghere merged 7 commits into
developmentfrom
53-backend---core---core-model-group-entity-implementation-2
Mar 11, 2025
Merged

53 backend core core model group entity implementation 2#180
thomasdejaeghere merged 7 commits into
developmentfrom
53-backend---core---core-model-group-entity-implementation-2

Conversation

@rvdkeere

@rvdkeere rvdkeere commented Mar 9, 2025

Copy link
Copy Markdown
Contributor

Implementation of the Group entity with a list of members (Student objects) and an add & remove member method instead of a setter to better conform to domain rules of clean architecture

@rvdkeere rvdkeere self-assigned this Mar 9, 2025
@rvdkeere rvdkeere added the enhancement New feature or request label Mar 9, 2025
@rvdkeere rvdkeere added this to the Milestone 1 milestone Mar 9, 2025
@rvdkeere rvdkeere linked an issue Mar 9, 2025 that may be closed by this pull request
2 tasks
changed attributes and functions to better conform to clean architecture

@lennertdr lennertdr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thoughts:

  1. Why keep full Student objects in a group? Thought we would minimize info by only using id's or fetching the data we need.

  2. Here the group-id is readonly and could be null, so when a group object is made without id (before creating it in the database), it can't be changed later. Can't this be a problem?

@rvdkeere

rvdkeere commented Mar 9, 2025

Copy link
Copy Markdown
Contributor Author

@lennertdr

  1. I based this approach on the idea that a group and its members are more closely tied together than, say, a student and their classes. It also more closely follows the principles of clean architecture by keeping the domain more object oriented. However if there are common use cases where only the group is needed, without accessing its members, then minimizing info could indeed be the better choice here.

  2. I don't see when this would be a problem. The moment you store the object for the first time it gets assigned an id, and any subsequent use case fetching the object will get that id with it. Are there any cases where an entity's id should be changed after creation? If not, it seems safer to keep it readonly.

@rvdkeere
rvdkeere requested a review from brieromb March 9, 2025 23:22

@brentjan brentjan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought we agreed that an entity should not have a direct list of other entities it has a relation with? But that an entity should have a list of id's?

replaced student objects with ids
@rvdkeere

Copy link
Copy Markdown
Contributor Author

Due to popular demand and to remain consistent with other entities, I replaced the list of student objects with a list of ids with a basic setter.

@rvdkeere
rvdkeere marked this pull request as ready for review March 10, 2025 14:09
@rvdkeere
rvdkeere requested review from brentjan and lennertdr March 10, 2025 14:32
@thomasdejaeghere
thomasdejaeghere merged commit 57d6f79 into development Mar 11, 2025
@thomasdejaeghere
thomasdejaeghere deleted the 53-backend---core---core-model-group-entity-implementation-2 branch March 11, 2025 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Backend] - Core - Core model Group entity Implementation

5 participants