Skip to content

Requests versioning 3#173

Open
oilagh wants to merge 13 commits intomainfrom
requests-versioning-2
Open

Requests versioning 3#173
oilagh wants to merge 13 commits intomainfrom
requests-versioning-2

Conversation

@oilagh
Copy link
Contributor

@oilagh oilagh commented Mar 14, 2026

Description

modifies existing request enpoints to fetch the request with the latest version (in case of same ids)

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • [ X ] New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (code improvement without changing functionality)
  • Documentation update
  • Configuration/infrastructure change
  • Performance improvement
  • Test coverage improvement

Related Issue(s)

Closes #
Related to # #108

What Changed?

  • Essentially just update find request, find requests, and inert request at the repo level to change the way it accesses records from the DB
  • Add a new column request_version to DB

Testing & Validation

I only modified existing tests for our requests endpoints and made sure they passed.

How this was tested

  1. Unit tests

Screenshots/Recordings

The below shows a POST request which makes a fresh request and 2 following PUT requests. As expected, the POST delivers a fresh id, created at, and request version. And the following PUT requests change the request version to date NOW() and maintains the created at and id of the request.

image

And finally I'll show you that GET request will serve up the latest version (timestamp) of the record stored

image

timestamp is 2026-03-17T21:47:13.232841-04:00 which aligns with the last PUT request we made

Unfinished Work & Known Issues

  • [ X] None, this PR is complete and production-ready

Notes & Nuances



Pre-Merge Checklist

Code Quality

  • Code follows the project's style guidelines and conventions
  • Self-review completed (I've reviewed my own code for obvious issues)
  • No debugging code, console logs, or commented-out code left behind
  • No merge conflicts with the base branch
  • Meaningful commit messages that explain the "why"

Testing & CI

  • All CI checks are passing
  • All new and existing tests pass locally
  • Test coverage hasn't decreased (or decrease is justified)
  • Linting passes without errors

Documentation

  • Code is self-documenting or includes helpful comments for complex logic
  • API documentation updated (if backend endpoints changed)
  • Type definitions are accurate and up-to-date

Reviewer Notes

  • Areas needing extra attention: ...
  • Questions for reviewers: ...

…, insert requests) and add migration to add column request_version to requests table and also make it so created_at which contains date created reflects the version because shud be the same
@vercel
Copy link

vercel bot commented Mar 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
selfserve-web Ready Ready Preview, Comment Mar 18, 2026 1:04am

@oilagh oilagh changed the title Requests versioning 2 Requests versioning Mar 17, 2026
@codecov
Copy link

codecov bot commented Mar 17, 2026

Codecov Report

❌ Patch coverage is 0% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 12.65%. Comparing base (5b0b85e) to head (004bc09).

Files with missing lines Patch % Lines
backend/internal/repository/requests.go 0.00% 24 Missing ⚠️
backend/internal/handler/requests.go 0.00% 14 Missing ⚠️
backend/internal/service/server.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #173       +/-   ##
===========================================
- Coverage   23.00%   12.65%   -10.36%     
===========================================
  Files          34       79       +45     
  Lines        1143     2505     +1362     
  Branches        0       24       +24     
===========================================
+ Hits          263      317       +54     
- Misses        875     2183     +1308     
  Partials        5        5               
Flag Coverage Δ
backend 22.51% <0.00%> (-0.50%) ⬇️
mobile 84.00% <ø> (?)
web 0.93% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
backend/internal/models/requests.go 0.00% <ø> (ø)
backend/internal/service/server.go 0.00% <0.00%> (ø)
backend/internal/handler/requests.go 88.32% <0.00%> (-10.06%) ⬇️
backend/internal/repository/requests.go 0.00% <0.00%> (ø)

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…/adjust cursor request endpoint to reflect the new updated schema
manueltorres0 and others added 3 commits March 17, 2026 21:02
* rooms migration file

* feat: guest booking migration

* rls fix

* indexes on both tables

* type fix

* incomplete guest bookings table + endpoint

* server integrated + openapi

* moved getRooms to rooms repo

* get guests endpoint

* changes

* fixed openapi

* remove stale webhook dir

* route for specific guest info

* cleanup GetBookingsByFloor handler

* changed some models

* filtered by hotel id

* lint

* may the lint gods be merciful

* comments

* fixed issue

* rename

---------

Co-authored-by: eric-kitagawa <kitagawa.e@northeastern.edu>
…, insert requests) and add migration to add column request_version to requests table and also make it so created_at which contains date created reflects the version because shud be the same
@oilagh oilagh changed the title Requests versioning Requests versioning 3 Mar 18, 2026
Copy link
Contributor

@Dao-Ho Dao-Ho left a comment

Choose a reason for hiding this comment

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

some comments but overall logic looks great! 🔥 🚀

return errs.BadRequest("request id is not a valid UUID")
}

var incoming models.MakeRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this var more descriptive

if err := c.BodyParser(&incoming); err != nil {
return errs.InvalidJSON()
}
req := models.Request{ID: id, MakeRequest: incoming}
Copy link
Contributor

Choose a reason for hiding this comment

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

same with this var, more descriptive please 🙏


if err := validateCreateRequest(&req); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we also have a validation system in place, let's remove this validateCreateRequest and use the validation Zaydaan added.

Please look at how BindAndValidate is used in our users.go file in the handler

if err := httpx.BindAndValidate(c, &CreateUserRequest); err != nil {
		return err
	}

`, req.HotelID, req.GuestID, req.UserID, req.ReservationID, req.Name,
if req.ID == "" {
req.ID = uuid.New().String()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be the CRUD's job to create a new ID, let's throw an error if this is the case. Should be on the caller for passing this id value

req.Status, req.Priority, req.EstimatedCompletionTime,
req.ScheduledTime, req.Notes).Scan(&req.ID, &req.CreatedAt, &req.UpdatedAt)
req.ScheduledTime, req.Notes).Scan(&req.ID, &req.CreatedAt, &req.RequestVersion)

Copy link
Contributor

Choose a reason for hiding this comment

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

niceee 🔥

ALTER COLUMN request_version DROP DEFAULT;

ALTER TABLE public.requests DROP COLUMN updated_at No newline at end of file
ALTER TABLE public.requests DROP COLUMN updated_at
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this to a new migration file - each new migration should be its own separate file.

Also, this should be it's own separate PR. Every migration should be a separate pr

ALTER TABLE public.requests DROP CONSTRAINT requests_pkey;

ALTER TABLE public.requests ADD CONSTRAINT requests_pkey PRIMARY KEY (id, request_version); No newline at end of file
ALTER TABLE public.requests ADD CONSTRAINT requests_pkey PRIMARY KEY (id, request_version);
Copy link
Contributor

Choose a reason for hiding this comment

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

same with this - separate PR and file





Copy link
Contributor

Choose a reason for hiding this comment

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

what is this file? I don't think you meant to commit this

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.

3 participants