Feature delete user - check active reservations by Vasilijez · Pull Request #34 · book-em/user-service

Vasilijez · 2025-10-11T21:28:02Z

Closes #6.

First, I'd like to note that the branch name is not ideal, as there's still more work to be done to complete the user deletion feature.

This PR aims to incorporate as many components as feasible within the project's current state.

For more information, check the related issue #6, there will be a series of linked PRs.

Caveat: If you see empty commits, it’s very likely caused by using the git cherry-pick command from an old branch and resolving many conflicts.

Part of:

…ions

I also replaced 'Pending' with 'Active' in `GetPendingGuestReservations`, as it seemed to provide more consistency with `GetActiveHostReservations`. *Tests also need to be updated.

I need to use it so the integration tests pass.

magley

Ok, two things:

1) My comments left in this PR review.

DTOs should not have gorm annotations since they're not DB models.
user-service should not ask room-service to fetch host reservations (it should ask reservation-service)

2) The missing commits.

I'm guessing this is because user-service's develop branch changed in the meantime and you wanted to fetch the new changes before creating this PR?

I had that situation yesterday after rebasing your PR into develop and then wanting to create my own PR. I didn't use cherry-picking, but instead:

[develop is updated]
git checkout my_branch
git rebase develop
git push --force-with-lease

I'm not sure if this would solve your problem, though. I just wanted to get the new commits from develop and then force push my local branch.

magley · 2025-10-12T08:00:21Z

+type ReservationDTO struct {
+	ID                 uint      `gorm:"primaryKey"`
+	RoomID             uint      `gorm:"not null"`
+	RoomAvailabilityID uint      `gorm:"not null"`
+	RoomPriceID        uint      `gorm:"not null"`
+	GuestID            uint      `gorm:"not null"`
+	DateFrom           time.Time `gorm:"not null"`
+	DateTo             time.Time `gorm:"not null"`
+	GuestCount         uint      `gorm:"not null"`
+	Cancelled          bool      `gorm:"not null"`
+	Cost               uint      `gorm:"not null"`
+}


Should a DTO have gorm annotations?

Not really. I accidentally left it there after copying Reservation from model.go in the reservation-service. It’s now removed as part of another issue you pointed out (f171d2e).

magley · 2025-10-12T08:05:31Z

 type ReservationDTO struct {
+	ID                 uint      `gorm:"primaryKey"`
+	RoomID             uint      `gorm:"not null"`
+	RoomAvailabilityID uint      `gorm:"not null"`
+	RoomPriceID        uint      `gorm:"not null"`
+	GuestID            uint      `gorm:"not null"`
+	DateFrom           time.Time `gorm:"not null"`
+	DateTo             time.Time `gorm:"not null"`
+	GuestCount         uint      `gorm:"not null"`
+	Cancelled          bool      `gorm:"not null"`
+	Cost               uint      `gorm:"not null"`
+}


Why does roomclient need to define ReservationDTO? Just use reservationclient/model.go's definition.

Ok, so I think it's because getActiveHostReservations is defined in roomclient.

So my question then becomes: why is room-service in charge of fetching hosts' reservations? Especially when the definition of room-service's GetActiveHostReservations() forwards the request to reservation-service.

Why not do the same here?

user-service -> reservation-service. Skip room-service as a middleman. If you need to fetch the rooms before you can get the reservations of a host, do that in reservation-service. So the flow would be like:

Ok, so I think it's because getActiveHostReservations is defined in roomclient.

Correct.

So my question then becomes: why is room-service in charge of fetching hosts' reservations? Especially when the definition of room-service's GetActiveHostReservations() forwards the request to reservation-service.

Why not do the same here?

That's a good question. It happened because I made a shift during development. Initially, for the host, my idea was to avoid sending too many requests and creating tight coupling, so I wanted to handle everything in a single request. My plan was to contact the room service, check each room for active reservations, and if none had any, delete all rooms. Later, I came across a different design sketch (yours) and shifted from my initial idea, which led to the issue you noticed.

Sorry for the poor "sequence diagram". It's just a rough sketch I made early on, before any implementation.

I'm not entirely sure what the best approach is now. I'll take some time to think about it, especially since I still need to implement room deletion.

You shouldn't worry too much about sending so many requests because:

It's a microservice app, so it "comes with the territory"

Since this is just a bunch of GET requests, we won't have any syncing issues (or, at least, none that we have to think about)

My issue with the current implementation is:

it's inconsistent between Guest and Host (guest talks to reservation-service directly while host doesn't).

reservations are not room-service's concern

there's duplicated logic and code (this might be the worst offender since if we ever change ReservationDTO, we have to remember to change it both in roomclient and reservationclient. If we had a 1000 clients, you'd have to check which ones define ReservationDTO every time, as opposed to knowing that ReservationDTO is found only in reservationclient).

implementation details are leaking into other services

I think the client part of each microservice should be treated as "private" to that microservice only.

Another way to think about this is: imagine if room-service and reservation-service were both some external API. Who would you "ask" whether some user has any reservations? As far as user-service is concerned, the actual details of how reservation-service "answers" that question is not important (black box).

Okay, I think I got what you meant, I hope I did it correctly this time (f171d2e).

Vasilijez · 2025-10-13T11:18:00Z

2) The missing commits.

I'm guessing this is because user-service's develop branch changed in the meantime and you wanted to fetch the new changes before creating this PR?

I had that situation yesterday after rebasing your PR into develop and then wanting to create my own PR. I didn't use cherry-picking, but instead:
[develop is updated]
git checkout my_branch
git rebase develop
git push --force-with-lease
I'm not sure if this would solve your problem, though. I just wanted to get the new commits from develop and then force push my local branch.

I usually follow your approach, but yesterday I intentionally used git cherry-pick. All of these changes were on an old branch named feature-delete-user-finish (which has been deleted in the meantime). Then I created a new branch, feature-delete-user-core, and performed the cherry-pick. I did it because many changes occurred across the entire codebase due to the new logging, and I wanted to avoid rebasing or modifying your commits in history, as that would have caused conflicts in almost every commit.
So for me, it was a better option to go commit by commit and resolve conflicts gradually. It was actually quite simple, but as you can see, a few commits ended up being empty. I assume that's because when resolving conflicts, I discarded all changes from the cherry-picked commit, resulting in no effective changes. I'll just need to be more careful next time not to push empty commits.

magley · 2025-10-13T11:19:41Z

-package roomclient
-
-import (
-	utils "bookem-user-service/util"
-	"context"
-	"encoding/json"
-	"fmt"
-	"io"
-	"net/http"
-)
-
-type RoomClient interface {
-	// GetActiveHostReservations finds all reservations made to rooms owned by
-	// `host` that haven't completed yet. The user must be a host.
-	GetActiveHostReservations(ctx context.Context, jwt string) ([]ReservationDTO, error)
-}
-
-type roomClient struct {
-	baseURL string


You shouldn't have deleted the entire file roomclient/client.go. We'll need RoomClient later on, it's ok if it's empty for now.

magley · 2025-10-13T11:35:59Z

I usually follow your approach, but yesterday I intentionally used git cherry-pick. All of these changes were on an old branch named feature-delete-user-finish (which has been deleted in the meantime). Then I created a new branch, feature-delete-user-core, and performed the cherry-pick. I did it because many changes occurred across the entire codebase due to the new logging, and I wanted to avoid rebasing or modifying your commits in history, as that would have caused conflicts in almost every commit.
So for me, it was a better option to go commit by commit and resolve conflicts gradually. It was actually quite simple, but as you can see, a few commits ended up being empty. I assume that's because when resolving conflicts, I discarded all changes from the cherry-picked commit, resulting in no effective changes. I'll just need to be more careful next time not to push empty commits.

I appreciate the effort but it's caused us more headaches than if you just copy-pasted the code that you need from the old branch. I mean I get that that's not a good idea and we should use cherry-picking but then stuff like this happens.

This is possible since `room-service` has been discarded (no effective changes), and `reservation-service` has already been merged.

magley

Good work.

Vasilijez added 13 commits October 11, 2025 16:30

feat: Add reservationclient to directly check active guest reservat…

81afc59

…ions

feat: Logged-in user deletes themselves

280d472

I also replaced 'Pending' with 'Active' in `GetPendingGuestReservations`, as it seemed to provide more consistency with `GetActiveHostReservations`. *Tests also need to be updated.

feat: Update baseURL of RoomClient

05692b3

feat: Finish GetActiveHostReservations within RoomClient

c400f2c

feat: Finish GetActiveHostReservations within RoomClient

be1cb14

test: Retune unit tests mostly due to ReservationClient

656fc88

test: Update unit tests for TestDelete

a663734

test: Add and update integration tests for TestIntegration_Delete

8f2d390

feat: Update reservation-client to use new logging

dde6a24

feat: Add forgotten error messages in reservation-client

5a3dfb3

test: Update unit tests with context.Background() parameter

dc43dd5

fix: Untar private key for CI

b0856f8

feat: Use branches with latest changes

e012f07

I need to use it so the integration tests pass.

Vasilijez self-assigned this Oct 11, 2025

Vasilijez added the feature New functional or non-functional requirement implementation. label Oct 11, 2025

This was referenced Oct 11, 2025

Feature delete user - check active reservations book-em/room-service#27

Closed

Feature delete user - check active reservations book-em/reservation-service#16

Merged

Vasilijez linked an issue Oct 11, 2025 that may be closed by this pull request

User account deletion #6

Closed

Vasilijez requested a review from magley October 11, 2025 21:38

magley requested changes Oct 12, 2025

View reviewed changes

feat: Move logic of GetActiveHostReservations to reservation-service

f171d2e

Vasilijez requested a review from magley October 13, 2025 11:18

magley requested changes Oct 13, 2025

View reviewed changes

Vasilijez added 2 commits October 13, 2025 13:50

feat: Add empty client.go file

3b867c1

feat: Use develop as latest branch from other microservices

9ee6461

This is possible since `room-service` has been discarded (no effective changes), and `reservation-service` has already been merged.

Vasilijez requested a review from magley October 13, 2025 12:00

feat: Inject RoomClient in service and tests

c28393b

magley approved these changes Oct 13, 2025

View reviewed changes

Vasilijez merged commit 0e006dd into develop Oct 13, 2025
2 checks passed

Conversation

Vasilijez commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magley left a comment

Choose a reason for hiding this comment

Uh oh!

magley Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

Vasilijez Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

magley Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

magley Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

Vasilijez Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

magley Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vasilijez Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Vasilijez commented Oct 13, 2025

Uh oh!

magley Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vasilijez Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

magley commented Oct 13, 2025

Uh oh!

magley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Vasilijez commented Oct 11, 2025 •

edited

Loading

magley Oct 12, 2025 •

edited

Loading

magley Oct 13, 2025 •

edited

Loading