Skip to content

Implementation logical database scheme - #17

Merged
thomasdejaeghere merged 27 commits into
developmentfrom
14-implement-logical-database-scheme
Feb 24, 2025
Merged

Implementation logical database scheme#17
thomasdejaeghere merged 27 commits into
developmentfrom
14-implement-logical-database-scheme

Conversation

@brentjan

Copy link
Copy Markdown
Contributor

First draft of the implementation of the logical database scheme. There are some changes needed to the presented database scheme in #2 .

Things yet to do:

  • Write code that closes and saves the database in a clean way when the server is manually shut down
  • Have some more concise logging when initializing/shutting down the database
  • See if there's a way to have the database username, password, ... in a seperate file instead of hardcoded constants
  • Test insertion and querying

@brentjan brentjan added this to the Milestone 1 milestone Feb 22, 2025
@brentjan

Copy link
Copy Markdown
Contributor Author

Insight: the way i initialize the database in index.ts is not how the Clean architecture works. We'll need to define interfaces for setting up the database connection and implement it for the PostgreSQL database.

@bramcomyn bramcomyn added the enhancement New feature or request label Feb 22, 2025
@bramcomyn bramcomyn linked an issue Feb 22, 2025 that may be closed by this pull request
@bramcomyn

Copy link
Copy Markdown
Contributor

First thing that comes to my mind is: why does he want to push to main already? 😉 Let's start by changing that into development :)

@bramcomyn
bramcomyn changed the base branch from main to development February 22, 2025 20:27
Consistency is key to great code

@bramcomyn bramcomyn 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 approve these changes, with the small detail I commented on. See below:

}

@Entity()
export class Invites {

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.

Wouldn't it be better to rename this to Invite? I know this might be a bit of a detail, but I feel like that name would suit better the purpose of this entity

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.

Zou de naam voor deze tabel niet beter iets zoals PendingJoinRequests zijn? Aangezien het nu eigenlijk de zender van de invite is die de geïnviteerde moet accepteren, en niet omgekeerd zoals je zou verwachten bij invites.

@brieromb
brieromb marked this pull request as ready for review February 23, 2025 10:35
email!: string

@Column({ type: "varchar", length: 50 })
forename!: string

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.

forename veld naar first_name hernoemen?

@brieromb brieromb 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.

Goed gedaan @brentjan! Ik heb nog 2 kleine opmerkingen geplaatst, maar voor de rest ziet dit er echt goed uit.

@JoinColumn({ name: "student_id" })
student!: Student

@OneToOne(() => QuestionThread)

@brentjan brentjan Feb 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should be able to be null if there isn't a thread yet? Same for answer.

Maybe with:

@OneToOne(() => AssignmentAnswer, { nullable: true })

But this might not be supported by PostgreSQL

@brentjan

Copy link
Copy Markdown
Contributor Author

Applied the comments from the reviews and also did some of the small todo's mentioned above.

Big change: the classes i defined previously are now in /backend/src/infrastructure/database/data/data_models. These classes are specific for TypeORM and should not be used in any business logic!

The clean-architecture way of working says we need to define general data entities (example in backend/src/core/entities), these are then used in business logic.

Ofcourse when we write queries with TypeORM we will need to map the TypeORM data models to entities as said in this article (under "Mapping").

Can @thomasdejaeghere and @bramcomyn check that this is indeed the case?
Can @brieromb help me implement these entities?

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 would change the name of this Interface to ITeacher to avoid long class names in the future.

@thomasdejaeghere

Copy link
Copy Markdown
Contributor

Applied the comments from the reviews and also did some of the small todo's mentioned above.

Big change: the classes i defined previously are now in /backend/src/infrastructure/database/data/data_models. These classes are specific for TypeORM and should not be used in any business logic!

The clean-architecture way of working says we need to define general data entities (example in backend/src/core/entities), these are then used in business logic.

Ofcourse when we write queries with TypeORM we will need to map the TypeORM data models to entities as said in this article (under "Mapping").

Can @thomasdejaeghere and @bramcomyn check that this is indeed the case? Can @brieromb help me implement these entities?

This is indeed true, In my last push to branch 11 I already set up the needed files for the Student entity, connection with the database then needs to implemented in UserRepositoryImpl.ts in /backend/src/infrastructure/repositories. Code looks good! I will merge this branch into dev.

@thomasdejaeghere
thomasdejaeghere merged commit eb9308a into development Feb 24, 2025
@thomasdejaeghere
thomasdejaeghere deleted the 14-implement-logical-database-scheme branch February 24, 2025 15: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.

Implement logical database scheme

5 participants