Skip to content

refactor!: disallow star rating < 1 - #27896

Merged
danieldietzler merged 9 commits into
mainfrom
refactor/nonnegative-rating
Jun 4, 2026
Merged

refactor!: disallow star rating < 1#27896
danieldietzler merged 9 commits into
mainfrom
refactor/nonnegative-rating

Conversation

@meesfrensel

@meesfrensel meesfrensel commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator

Description

The breaking change counterpart to #26357

Including migrating to convert -1 to null in the DB.

Caution

Breaking Change
Accepted ratings for assets are changed from [-1, 5] | null to [1, 5] | null.

includes the mobile side migration (asset rating and search)

Please describe to which degree, if any, an LLM was used in creating this pull request.

None

@meesfrensel
meesfrensel force-pushed the refactor/nonnegative-rating branch from 7c01bd4 to 2325a35 Compare April 17, 2026 13:57
@YarosMallorca

Copy link
Copy Markdown
Collaborator

Currently this breaks the mobile implementation of star rating.
Maybe once #27231 gets merged, the implementation can be adjusted to accept optional values.

description: z.string().nullish().default(null).describe('Image description'),
projectionType: z.string().nullish().default(null).describe('Projection type'),
rating: z.number().nullish().default(null).describe('Rating'),
rating: z.int().min(1).max(5).nullish().default(null).describe('Rating'),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As a response dto this should be fine, it only describes the range the server can return.

@meesfrensel
meesfrensel force-pushed the refactor/nonnegative-rating branch from 1424b08 to f16a83f Compare April 23, 2026 13:13
Comment thread server/src/dtos/search.dto.ts
@meesfrensel
meesfrensel marked this pull request as ready for review April 29, 2026 14:31
@danieldietzler

Copy link
Copy Markdown
Member

What's the plan with this? Aren't we waiting for open API to release the PR?

@meesfrensel

Copy link
Copy Markdown
Collaborator Author

I don't know, there has not been any activity on Timon's PR and this one is ready, we could later update the valid range from [0, 5] to [1, 5] in a new PR.

@timonrieger

timonrieger commented May 3, 2026

Copy link
Copy Markdown
Collaborator

I don't know, there has not been any activity on Timon's PR and this one is ready, we could later update the valid range from [0, 5] to [1, 5] in a new PR.

#27231 is blocked by an open PR in upstream in the generator. we can go ahead with this PR meanwhile (although would mean another breaking change technically when updating the range from 0 to 1 as min)

EDIT: i'd not merge this yet, since it does not add real value before lifting the lower boundary to 1 finally

@timonrieger

Copy link
Copy Markdown
Collaborator

@meesfrensel #27231 has been merged, so this PR is unblocked now. We should now be able to constrain on [1, 5] | null

@danieldietzler
danieldietzler force-pushed the refactor/nonnegative-rating branch from f7dbdbb to 7d23160 Compare June 4, 2026 12:14
@immich-push-o-matic

immich-push-o-matic Bot commented Jun 4, 2026

Copy link
Copy Markdown

📱 Android release APK (universal)a0f27f21ffb9f7c46c9492d41b89b3169525e202

Download: https://github.com/immich-app/immich/actions/runs/26966539494/artifacts/7417931103

QR code QR code

Installs as a separate app (applicationId app.alextran.immich.pr27896), so it coexists with the Play Store version and any other PR builds.

@timonrieger

Copy link
Copy Markdown
Collaborator

@shenlong-tanwen @danieldietzler I migrated the mobile call-sites now. As discussed we keep using Option internally and map to Optional only on the API boundary. Also added tests, although I am not sure how valuable they are (in the future) 😅 . They basically just prove that the migration works. Feel free to revert that commit!

and as Mees pointed out, the migration files needs a rename

@danieldietzler
danieldietzler enabled auto-merge (squash) June 4, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants