Skip to content

Feat/dto models#167

Merged
GaballaGit merged 11 commits intomainfrom
feat/DTO-Models
Mar 7, 2026
Merged

Feat/dto models#167
GaballaGit merged 11 commits intomainfrom
feat/DTO-Models

Conversation

@GaballaGit
Copy link
Member

This PR introduces DTO models for the API. Currently there are 2 different ones, request and response. At the moment they are both similar, with the only difference of request having an extra Update Model since many more fields don't need to be inputted for an update. Response DTO model might be updated later depending on what we want the user to receive.

This PR also makes make check and make test a little more pretty.

@GaballaGit GaballaGit requested a review from TheJolman March 1, 2026 23:26
@GaballaGit GaballaGit requested a review from sidvasu as a code owner March 1, 2026 23:26
@github-actions
Copy link

github-actions bot commented Mar 1, 2026

godoc reference preview ready ✨
Go Documentation

@GaballaGit
Copy link
Member Author

@TheJolman

Copy link
Collaborator

@TheJolman TheJolman left a comment

Choose a reason for hiding this comment

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

models look clean. TBH I don't understand adding go build to the check step, can you revert that commit to keep this PR focused? Also can you check out https://github.com/go-playground/validator ? I think it would be very useful here. Gonna test this a little more locally and get back to you with more feedback.

Copy link
Collaborator

@TheJolman TheJolman left a comment

Choose a reason for hiding this comment

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

Also can you organize it like internal/dto/announcements.go and have both request/response dtos in the same file just to reduce the amount of files that we're going to have. When doing this you should name them like UpdateAnnouncementRequest and UpdateAnnouncementResponse to avoid name conflicts (I think the current naming would create some). Also I dont think you need to create a base Announcement or Officer dto for now, as the response payloads should contain all fields anyways. You can keep them if you want, just rename them to AnnouncementDto or something bc the generic Announcement will likely create naming conflicts.

@GaballaGit
Copy link
Member Author

holy shit what did I do

@TheJolman
Copy link
Collaborator

holy shit what did I do

dude idk its fucked tho lol

@TheJolman
Copy link
Collaborator

Can you also implement these new DTOs in at least one handler? I don't think it will make this too big and will let us verify they actually work. @GaballaGit

@GaballaGit
Copy link
Member Author

I gotchu

@GaballaGit
Copy link
Member Author

Okay now implementing in, I don't know if I make a handler use dto model in this pr @TheJolman . My reasoning is that, currently our handler calls a service that depends on a db model, that itself isnt bad, but then the service calls the database layer with the dbmodel, meaning that the repository layer must be implemented to test this, and with this also comes the mapper. I have tested the dto models in the other PR #159, where announcements has achieved functionality with DTO, Repository, and Domain models all together.

@TheJolman
Copy link
Collaborator

Okay now implementing in, I don't know if I make a handler use dto model in this pr @TheJolman . My reasoning is that, currently our handler calls a service that depends on a db model, that itself isnt bad, but then the service calls the database layer with the dbmodel, meaning that the repository layer must be implemented to test this, and with this also comes the mapper. I have tested the dto models in the other PR #159, where announcements has achieved functionality with DTO, Repository, and Domain models all together.

What I had in mind is having the handlers use the DTOs and then just translating them to dbmodels before calling the service. This isn't going to be the final form obv but it keeps it fairly atomic.

@TheJolman TheJolman closed this Mar 6, 2026
@TheJolman TheJolman deleted the feat/DTO-Models branch March 6, 2026 17:06
@GaballaGit
Copy link
Member Author

@TheJolman what happened?

@TheJolman TheJolman restored the feat/DTO-Models branch March 6, 2026 17:18
@TheJolman TheJolman reopened this Mar 6, 2026
@TheJolman
Copy link
Collaborator

@TheJolman what happened?

mb was deleting stale branches from my cli and accidentally hit this

Copy link
Collaborator

@TheJolman TheJolman left a comment

Choose a reason for hiding this comment

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

looks good ty!

@GaballaGit GaballaGit merged commit 5826afc into main Mar 7, 2026
2 checks passed
@GaballaGit GaballaGit deleted the feat/DTO-Models branch March 7, 2026 00:26
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.

2 participants