Feature/fixes#191
Open
Isti01 wants to merge 9 commits into
Open
Conversation
📝 WalkthroughWalkthroughRefactors most Spring components from field-based ChangesSingle cohesive refactor (DI, entity fetching, auth, templates, build)
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Auth as Authentication
participant UserRepo as UserRepository
participant Service
participant Repo as EntityRepository
Client->>Controller: HTTP request (with auth)
Controller->>Auth: extract auth
Auth->>UserRepo: lookup user (getUser(userRepo))
UserRepo-->>Auth: UserEntity?
Auth-->>Controller: UserEntity?
alt user found
Controller->>Service: call business operation with user
Service->>Repo: findByIdOrNull(id)
Repo-->>Service: Entity? (nullable)
Service-->>Controller: result / error
Controller-->>Client: 200 OK / payload
else user missing
Controller-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/test/kotlin/hu/kirdev/schpincer/service/CancelOrderTest.kt (1)
1-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemoved test coverage for
updateDetailsand the composite end-to-end case.Per the AI summary, the test that validated
updateDetailslogic and the composite test at the end of the file were removed. Given thatupdateDetailsnow also performs a repository lookup (timeWindowRepo.findByIdOrNull(order.intervalId)!!) and is a likely NPE site, dropping its test reduces confidence in the very paths this PR is changing. Consider re-adding a focused test that mockstimeWindowRepoand asserts both the success and missing-time-window paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/kotlin/hu/kirdev/schpincer/service/CancelOrderTest.kt` around lines 1 - 256, Re-add focused unit tests covering CancelOrderProcedure.updateDetails and a composite end-to-end cancel flow: mock timeWindowRepo.findByIdOrNull(order.intervalId) to return a TimeWindowEntity for the success case and to return null to assert the NPE/FailedOrderException path, and assert that updateDetails invokes the repository lookup and updates procedure.timeWindow and order fields as expected; specifically target CancelOrderProcedure.updateDetails and the composite flow that calls loadOrder(), loadOpening(), loadTimeWindow()/findByIdOrNull, updateRemainingCounts(), updateExtraCategories() and updateDetails() to ensure the new repository lookup is covered.src/main/kotlin/hu/kirdev/schpincer/service/ReviewService.kt (1)
41-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAwkward null handling around
order?.openingId!!.With the migration to nullable lookups,
orders.getOne(orderId)andcircles.getOne(circleId)likely return nullable types. The current expressionorder?.openingId!!is contradictory: iforderisnull, the safe-call returnsnulland!!then throws an NPE — defeating the purpose of the safe-call. Either fail fast with a clear error if any prerequisite is missing, or short-circuit the review creation.🛡️ Suggested cleanup
- val circleId = orders.getCircleIdByOrderId(orderId)!! - val order = orders.getOne(orderId) + val circleId = orders.getCircleIdByOrderId(orderId) + ?: error("Order $orderId has no associated circle") + val order = orders.getOne(orderId) + ?: error("Order $orderId not found") val fullReview = ReviewEntity( circle = circles.getOne(circleId), order = order, - openingFeeling = openings.getOne(order?.openingId!!)?.feeling, + openingFeeling = openings.getOne(order.openingId!!)?.feeling,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/service/ReviewService.kt` around lines 41 - 47, The null-handling around order?.openingId!! is unsafe and contradictory; update ReviewService so you explicitly validate prerequisite lookups (orders.getOne(orderId), circles.getOne(circleId), openings.getOne(...)) before constructing the ReviewEntity: retrieve order and circle into non-null vals using the Elvis operator (or return/throw a clear exception) and then use order.openingId and openings.getOne(...) safely (also validate the opening result), so the ReviewEntity constructor never relies on a silent safe-call followed by !! that can produce unclear NPEs.src/main/kotlin/hu/kirdev/schpincer/web/ReviewController.kt (1)
55-60:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRequire order ownership before accepting a review.
This guard only checks that the caller is logged in and the order is reviewable. It never verifies that
orderIdbelongs to the authenticated user, so any signed-in user can submit a review for another shipped order by guessing its ID. Resolve the user null-safely and reject when the order owner does not match; the same check should gate the GET handler so the form is not exposed either.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/web/ReviewController.kt` around lines 55 - 60, The current POST handler uses orders.getOne(orderId) and only checks auth.hasUser(), order.status, and order.reviewId before calling reviews.createReview; change this to safely resolve the authenticated user via auth.getUser(userRepository) (null-check) and verify the fetched order's owner identifier (e.g., order.userId / order.ownerId) equals the authenticated user's id before allowing review creation—if not, throw an unauthorized/forbidden exception; apply the same ownership check to the GET handler that renders the review form so the form is only exposed to the order owner.src/main/kotlin/hu/kirdev/schpincer/web/ConfigureController.kt (1)
405-435:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCheck that the edited item/opening actually belongs to
circleId.These POST handlers authorize the path
circleId, then save the loadeditemId/openingIdwithout verifying the record belongs to that circle. Someone who can edit circle A can submit an ID from circle B and modify it. Mirror the membership check already used ineditMemberanddeleteOpeningConfirmbefore saving.Also applies to: 514-528
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/web/ConfigureController.kt` around lines 405 - 435, The handler currently loads the target entity via items.getOne(itemId) and then saves it without verifying it belongs to the path's circleId, allowing cross-circle edits; before mutating/saving (after retrieving original) add a membership/ownership check (e.g. compare original.circle.id or original.circleId to the incoming circleId or use the same helper logic as cannotEditCircle/editMember/deleteOpeningConfirm) and if it does not match, return a redirect like "redirect:/configure/$circleId?error=invalidPermissions"; apply the same ownership check to the other similar POST block (the opening handler referenced at lines 514-528) so both items and openings are verified to belong to the specified circle before saving.src/main/kotlin/hu/kirdev/schpincer/web/KitchenViewController.kt (1)
239-244:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate
orderIdagainst the selected opening before mutating it.Both handlers authorize the
{circleId, openingId}pair, but then update whateverorderIdis passed. A user who can edit one kitchen view can change the status or chef comment of an order from another opening/circle by swapping the ID. Load the order and confirm it belongs to the same opening/circle before callingupdateStatusorupdateChefComment.Also applies to: 258-263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/web/KitchenViewController.kt` around lines 239 - 244, Load the Order entity by orderId from the orders repository (e.g. orders.findById / orders.get) and verify its openingId/circleId matches the requested openingId/circleId (or use the existing openings.isCircleMatches check against the order's opening) before calling orders.updateStatus or orders.updateChefComment; if the loaded order is null or its opening/circle doesn't match, return the redirect without mutating. Apply this same validation to both the status handler (orders.updateStatus) and the chef-comment handler (orders.updateChefComment).
🧹 Nitpick comments (6)
src/main/kotlin/hu/kirdev/schpincer/config/GlobalExceptionControllerAdvice.kt (1)
20-20: ⚡ Quick winUse a lower log level for expected 404 traffic.
Line 20 logs every missing static resource as
error, which can flood error monitoring from routine probes/bot traffic.Suggested tweak
- logger.error("No static resource {} for request '{}'", ex.resourcePath, ex.message) + logger.warn("No static resource {} for request '{}'", ex.resourcePath, ex.message)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/config/GlobalExceptionControllerAdvice.kt` at line 20, In GlobalExceptionControllerAdvice replace the current error-level log in the static resource 404 handler (the line using logger.error("No static resource {} for request '{}'", ex.resourcePath, ex.message)) with a lower level (e.g., logger.debug or logger.info) so routine missing-resource requests don't generate error noise; update the call site in the same method to use the chosen lower log method and keep the same message and placeholders so context (ex.resourcePath and ex.message) is preserved.src/main/kotlin/hu/kirdev/schpincer/service/CircleService.kt (1)
51-54: 💤 Low valueSimplify with
firstOrNull().The empty-check + indexed access can be replaced with the idiomatic Kotlin helper, which is also safer if
repo.findAllByAliasever returns an immutable empty list of a non-Listtype.♻️ Proposed refactor
`@Transactional`(readOnly = true) open fun findByAlias(alias: String): CircleEntity? { - val results = repo.findAllByAlias(alias) - return if (results.isEmpty()) null else results[0] + return repo.findAllByAlias(alias).firstOrNull() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/service/CircleService.kt` around lines 51 - 54, In CircleService.findByAlias replace the manual empty-check + indexed access on results from repo.findAllByAlias(alias) with the idiomatic Kotlin firstOrNull() (e.g. use repo.findAllByAlias(alias).firstOrNull() or call firstOrNull() on the results variable) to simplify the code and avoid unsafe indexing; update the function to return that firstOrNull() result.src/test/kotlin/hu/kirdev/schpincer/service/CancelOrderTest.kt (1)
36-48: 💤 Low valueMock target still works, but consider mocking the actual extension call site.
findByIdOrNullis a Kotlin extension that internally delegates tofindById(id).orElse(null), so stubbingfindByIdhere correctly drivesloadOrder(). This is fine, but it couples the test to the extension's internal implementation. StubbingfindByIdOrNulldirectly (or usingorg.mockito.kotlin.any()matchers) would be more explicit about what the procedure actually calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/kotlin/hu/kirdev/schpincer/service/CancelOrderTest.kt` around lines 36 - 48, The test stubs orderRepository.findById(...) but the production call uses the Kotlin extension findByIdOrNull, so update the mock to target the actual call site: replace the whenever(orderRepository.findById(4)) stub in CancelOrderTest with a stub for orderRepository.findByIdOrNull(4) (or use org.mockito.kotlin.any() matchers) so that CancelOrderProcedure.loadOrder() is driven explicitly by the same method it calls; keep the same returned Optional/nullable value and assertions around procedure.order.src/main/kotlin/hu/kirdev/schpincer/web/DeployController.kt (1)
9-13: 💤 Low valueLGTM with a minor suggestion.
Constructor injection for
UserServicelooks good. For consistency, you could also movesystemAdminsinto the constructor (Spring supports@Valueon constructor parameters), eliminating thelateinit var:-open class DeployController(private val users: UserService) { - - `@Value`("\${schpincer.sysadmins:}") - private lateinit var systemAdmins: String +open class DeployController( + private val users: UserService, + `@Value`("\${schpincer.sysadmins:}") private val systemAdmins: String, +) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/web/DeployController.kt` around lines 9 - 13, Move the `@Value-injected` field into the class constructor to avoid the lateinit var: change DeployController to accept systemAdmins as a constructor parameter annotated with `@Value`("${schpincer.sysadmins:}") alongside the existing users: UserService parameter so the class uses constructor injection for both (replace the private lateinit var systemAdmins: String with a constructor parameter, keeping the existing users property and annotations).src/main/kotlin/hu/kirdev/schpincer/service/UserService.kt (1)
14-22: ⚡ Quick win
getByIdandgetByIdOrNullare now functionally identical.Both methods return
UserEntity?and look up by ID. Having two identically-behaving public APIs invites confusion at call sites about which to use. Consider consolidating to a single method (e.g., keepgetByIdOrNulland removegetById, or vice versa) and migrate the few remaining callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/service/UserService.kt` around lines 14 - 22, The two public methods getById and getByIdOrNull in UserService are identical; remove getById to avoid confusion and keep getByIdOrNull as the single source of truth: delete the open fun getById(uid: String): UserEntity? implementation that calls repo.findByIdOrNull(uid), update all callers to use getByIdOrNull(uid) instead (or add a small delegating deprecated wrapper if needed for a transitional period), ensure any interface/consumer signatures are updated, run tests and compile to catch missed call sites, and keep repo.findById/findByIdOrNull usage consistent within the class.src/main/kotlin/hu/kirdev/schpincer/web/MainController.kt (1)
76-84: 💤 Low valueConfirm
"error"resolves to a real view template.Returning the string
"error"when an alias is not found relies on a Thymeleaf template namederror.html(or a configured error mapping) being present. If not, this branch will produce a 500 instead of a clean error page. Also consider returning a proper HTTP status (e.g., 404) rather than rendering an error page with a 200 status, for better client semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/hu/kirdev/schpincer/web/MainController.kt` around lines 76 - 84, The branch that handles a missing alias currently returns the literal view name "error" (see circles.findByAlias(circle) ?: return "error"), which may not map to a real template and returns a 200 status; instead, either ensure an error.html template exists or (preferred) replace the early return with a proper 404 by throwing a ResponseStatusException(HttpStatus.NOT_FOUND, "Circle not found") (or return a ResponseEntity with 404) so that the controller method (where circle.matches(...) and circles.findByAlias(...) live) produces a correct HTTP 404 when the alias is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pom.xml`:
- Around line 123-127: The micrometer BOM declaration (groupId "io.micrometer",
artifactId "micrometer-bom", version "1.16.5", type "pom") is currently in
<dependencies> and missing scope="import"; move this dependency entry into the
<dependencyManagement> section and add <scope>import</scope> so Maven treats it
as a BOM and applies its version constraints; update or create the
<dependencyManagement> block if missing and place the micrometer-bom POM
dependency there with type "pom" and scope "import".
In `@src/main/kotlin/hu/kirdev/schpincer/config/SecurityConfig.kt`:
- Line 55: The security config currently restricts only "/profile" but leaves
the kitchen-view stats endpoints publicly accessible; update the auth matchers
in SecurityConfig (the auth.requestMatchers(...) configuration that uses
Role.USER and anyRequest().permitAll()) to either include the kitchen-view paths
(e.g., add "/kitchen-view/**" and "/api/kitchen-view/**" to the Role.USER
matcher alongside "/profile") so those endpoints remain authenticated, or remove
all references and handlers for the stats endpoints from KitchenViewController,
templates, and kitchen-view.js if you intend to delete the feature; modify the
code in SecurityConfig and then verify KitchenViewController.kt and UI templates
consistently reflect the chosen approach.
In `@src/main/kotlin/hu/kirdev/schpincer/service/CancelOrderProcedure.kt`:
- Around line 34-36: Replace the unsafe non-null assertions with explicit domain
errors: in loadOrder(), change the usage of orderRepository.findByIdOrNull(id)!!
to use the Elvis operator and throw a FailedOrderException with a clear message
if null; do the same for openings.getOne(order.openingId!!) by first
null-checking order.openingId and for
timeWindowRepo.findByIdOrNull(order.intervalId)!!—each missing entity should
result in throw FailedOrderException("...") instead of letting a Kotlin NPE
occur so that loadOrder(), the openings lookup, and the timeWindow lookup all
produce domain-meaningful errors.
In `@src/main/kotlin/hu/kirdev/schpincer/service/ChangeOrderProcedure.kt`:
- Around line 28-30: Replace the unsafe non-null assertion in
ChangeOrderProcedure.loadOrder() (and the equivalent loadOrder() in
CancelOrderProcedure) so that when orderRepository.findByIdOrNull(id) returns
null you throw a FailedOrderException with the appropriate domain response code
instead of letting a Kotlin NPE bubble up; locate both loadOrder() methods,
remove the "!!" usage and construct/throw FailedOrderException carrying the
domain response code used by other validation steps so missing/deleted/invalid
ids produce a consistent failure type.
In `@src/main/kotlin/hu/kirdev/schpincer/service/CircleMemberService.kt`:
- Around line 10-20: CircleMemberService.getOne now returns a nullable
CircleMemberEntity but callers in ConfigureController still use unsafe !!
unwrapping; change those calls to handle nulls safely (for example replace val
original = members.getOne(id)!! with val original = members.getOne(id) ?: return
"redirect:..." and similarly replace val cme = members.getOne(memberId)!! with
val cme = members.getOne(memberId) ?: return "redirect:..."), or use safe
navigation (?.) where appropriate to match the null-handling pattern already
used in the controller at other sites.
In `@src/main/kotlin/hu/kirdev/schpincer/service/CircleService.kt`:
- Around line 41-43: getOne in CircleService (and equivalent OpeningService) now
returns nullable; update all callsites that use getOne(...)!! (AdminController,
KitchenViewController, ConfigureController, CancelOrderProcedure) to handle
nulls explicitly instead of force-unwrapping: check the result of getOne(id)
with the Elvis operator or an if-null branch and return the appropriate 404
error view or redirect with error parameters (mirror
MainController.findByAlias's ?: handling), and for CancelOrderProcedure ensure
order.openingId is checked before using it and handle missing openingId/opening
by aborting/returning an error flow rather than allowing a NullPointerException.
In `@src/main/kotlin/hu/kirdev/schpincer/service/MakeOrderProcedure.kt`:
- Line 57: Replace the unsafe force-unwraps on itemsRepo.findByIdOrNull(id) with
a guarded null check inside MakeOrderProcedure: when the lookup returns null,
throw a FailedOrderException with an appropriate response code (use the
project’s item-not-found response constant or RESPONSE_INTERNAL_ERROR if no
specific constant exists) instead of letting a Kotlin NPE propagate; apply the
same fix for both occurrences where item is assigned from
itemsRepo.findByIdOrNull(id) so all unknown-item cases are translated into
controlled FailedOrderException errors.
In `@src/main/kotlin/hu/kirdev/schpincer/service/OrderService.kt`:
- Around line 95-98: In getCircleIdByOrderId replace the forced-nonnull chain so
missing data propagates as null: stop using orderEntity.openingId!! and the
trailing !! on .id — call
openingRepo.findByIdOrNull(orderEntity.openingId)?.circle?.id inside the
order.map and return that orElse(null); also review any callers such as
ReviewService.submitReview that currently assume a non-null return and adjust
their !! usages accordingly.
In `@src/main/kotlin/hu/kirdev/schpincer/service/SchPincerOidcUserService.kt`:
- Around line 27-29: The code currently calls
userService.exists(schPincerUser.internalId) and then calls
userService.getById(schPincerUser.internalId)!! which risks a race and a
forced-null crash; change the flow to perform a single read (e.g., call getById
or a findById method once), check the returned user for null, and only then
update fields like user.email = schPincerUser.email and save; remove the use of
the non-null assertion (!!) and handle the absent-user case explicitly (create
or return/throw as appropriate) within SchPincerOidcUserService.
In `@src/main/kotlin/hu/kirdev/schpincer/service/UserService.kt`:
- Around line 15-17: getById was changed to return nullable UserEntity?, so
replace all force-unwraps with explicit null handling: in ConfigureController
where code does users.getById(it.userId)!! inside a .map(), guard against
missing users by filtering out nulls or returning a 404/meaningful error for the
whole request instead of !!; in SchPincerOidcUserService, eliminate the TOCTOU
by loading the user once and handling a null (e.g., use userService.getById(...)
and if null throw/return a controlled authentication error or load-and-create
within a single transactional/atomic operation), do not rely on a prior exists()
check followed by !!; in UserService.setRoom replace getById(userId)!! with an
explicit null check and return an error/throw (or document and enforce a
non-null contract) so callers cannot silently violate the contract. Ensure each
change logs or returns appropriate errors instead of NPEs and reference the
methods getById, setRoom, and the call sites in ConfigureController and
SchPincerOidcUserService when making the fixes.
In `@src/main/kotlin/hu/kirdev/schpincer/web/AdminController.kt`:
- Around line 217-220: The changeCard handler currently calls
CardType.valueOf(card) which throws IllegalArgumentException for invalid inputs;
update the changeCard function to catch IllegalArgumentException (or validate
the input) around the CardType.valueOf(card) call and on error return "failed"
instead of letting the exception propagate; ensure valid enum values are
assigned to user.pekCardType and saved via users.save(user) only on success,
keeping the existing auth.getUser(userRepository) check and return behavior.
- Around line 167-169: The GET handler currently dereferences the nullable
result of users.getByIdOrNull(uid) with user!! which can throw; change the GET
handler to mirror the POST handler's null-check by guarding the result (from
users.getByIdOrNull) and returning "redirect:/admin/?error=invalidUid" when
null, before calling model.addAttribute("name", ...) and
model.addAttribute("sysadmin", ...); update any references to user in that
method accordingly to use the non-null local after the guard.
In `@src/main/kotlin/hu/kirdev/schpincer/web/ApiController.kt`:
- Around line 62-64: In the four endpoints getItem, getAllItems,
getAllItemsToday and getAllItemsTomorrow replace the current loggedIn check
(auth?.isAuthenticated == true) with a concrete-user check such as
auth.getUser(userRepository) != null or auth.getUserId() != null so anonymous
AuthenticationTokens aren’t treated as logged-in; update the loggedIn variable
in each of those methods accordingly so the existing visibleWithoutLogin
filtering logic (used just after those assignments) correctly distinguishes real
users from anonymous requests.
In `@src/main/kotlin/hu/kirdev/schpincer/web/ConfigureController.kt`:
- Around line 204-205: The code currently force-unwraps nullable lookups (e.g.
members.getOne(id)!! and original.circle!!.id) which causes 500s for
missing/stale records; replace each `!!` usage with null-safe checks that
redirect on invalid IDs (e.g. use `members.getOne(id) ?: return
"redirect:/configure/$circleId?invalidId"` and similarly ensure
`original.circle` is non-null before accessing its id, returning the same
redirect when null). Apply the same pattern for all other occurrences called out
(the other getOne()/circle accesses around the indicated ranges) so every lookup
boundary returns a redirect on null instead of throwing.
In `@src/main/kotlin/hu/kirdev/schpincer/web/Utility.kt`:
- Around line 56-64: The functions cannotEditCircle and cannotEditCircleNoPR use
fragile positional destructuring from auth.getUser(users); replace that with a
null-checked local variable and explicit named property access (e.g., val user =
auth.getUser(users) ?: return true; val sysadmin = user.sysadmin; val
permissions = user.permissions) and then use those names in the existing
permission logic so the code is robust to changes in UserEntity.
In `@src/main/resources/static/js/item.js`:
- Line 18: The URL builder assembles query params incorrectly by using (filter ?
'&' : '?'), producing strings like "api/items/now&circle=..."; update the
construction for the const url so you first build the base = 'api/items' +
filter and then choose the separator based on whether the base already contains
a '?' (use '&' if it does, otherwise '?') before appending 'circle=' (only when
profile !== 0). Apply the same fix to the other occurrence referenced (line ~81)
so both places use base.indexOf('?')/includes('?') to decide between '?' and '&'
when adding query parameters.
- Line 18: The frontend is appending the circle query param for all endpoints
(see url construction in appendNext()), but the ApiController methods that
handle /api/items/now and /api/items/tomorrow do not accept a circle
RequestParam; update appendNext() so it only adds &circle=... when the base
endpoint is the paginated /api/items list (i.e., when filter is empty or refers
to the list endpoint) and explicitly skip appending circle for paths containing
"/now" or "/tomorrow" (or alternatively detect whether the request path equals
'/api/items'); locate the url construction in appendNext() and conditionally
concatenate the circle param only for the list handler.
- Line 81: The construction of the request URL in the const url declaration uses
encodeURI(keyword) which does not percent-encode reserved query characters;
update both occurrences (the q= parameter building in the const url at the top
and the duplicate at line ~100) to use encodeURIComponent(keyword) so
ampersands, equals, hashes and other reserved chars in the keyword are safely
encoded; search for the const url string and replace encodeURI(keyword) with
encodeURIComponent(keyword).
---
Outside diff comments:
In `@src/main/kotlin/hu/kirdev/schpincer/service/ReviewService.kt`:
- Around line 41-47: The null-handling around order?.openingId!! is unsafe and
contradictory; update ReviewService so you explicitly validate prerequisite
lookups (orders.getOne(orderId), circles.getOne(circleId), openings.getOne(...))
before constructing the ReviewEntity: retrieve order and circle into non-null
vals using the Elvis operator (or return/throw a clear exception) and then use
order.openingId and openings.getOne(...) safely (also validate the opening
result), so the ReviewEntity constructor never relies on a silent safe-call
followed by !! that can produce unclear NPEs.
In `@src/main/kotlin/hu/kirdev/schpincer/web/ConfigureController.kt`:
- Around line 405-435: The handler currently loads the target entity via
items.getOne(itemId) and then saves it without verifying it belongs to the
path's circleId, allowing cross-circle edits; before mutating/saving (after
retrieving original) add a membership/ownership check (e.g. compare
original.circle.id or original.circleId to the incoming circleId or use the same
helper logic as cannotEditCircle/editMember/deleteOpeningConfirm) and if it does
not match, return a redirect like
"redirect:/configure/$circleId?error=invalidPermissions"; apply the same
ownership check to the other similar POST block (the opening handler referenced
at lines 514-528) so both items and openings are verified to belong to the
specified circle before saving.
In `@src/main/kotlin/hu/kirdev/schpincer/web/KitchenViewController.kt`:
- Around line 239-244: Load the Order entity by orderId from the orders
repository (e.g. orders.findById / orders.get) and verify its openingId/circleId
matches the requested openingId/circleId (or use the existing
openings.isCircleMatches check against the order's opening) before calling
orders.updateStatus or orders.updateChefComment; if the loaded order is null or
its opening/circle doesn't match, return the redirect without mutating. Apply
this same validation to both the status handler (orders.updateStatus) and the
chef-comment handler (orders.updateChefComment).
In `@src/main/kotlin/hu/kirdev/schpincer/web/ReviewController.kt`:
- Around line 55-60: The current POST handler uses orders.getOne(orderId) and
only checks auth.hasUser(), order.status, and order.reviewId before calling
reviews.createReview; change this to safely resolve the authenticated user via
auth.getUser(userRepository) (null-check) and verify the fetched order's owner
identifier (e.g., order.userId / order.ownerId) equals the authenticated user's
id before allowing review creation—if not, throw an unauthorized/forbidden
exception; apply the same ownership check to the GET handler that renders the
review form so the form is only exposed to the order owner.
In `@src/test/kotlin/hu/kirdev/schpincer/service/CancelOrderTest.kt`:
- Around line 1-256: Re-add focused unit tests covering
CancelOrderProcedure.updateDetails and a composite end-to-end cancel flow: mock
timeWindowRepo.findByIdOrNull(order.intervalId) to return a TimeWindowEntity for
the success case and to return null to assert the NPE/FailedOrderException path,
and assert that updateDetails invokes the repository lookup and updates
procedure.timeWindow and order fields as expected; specifically target
CancelOrderProcedure.updateDetails and the composite flow that calls
loadOrder(), loadOpening(), loadTimeWindow()/findByIdOrNull,
updateRemainingCounts(), updateExtraCategories() and updateDetails() to ensure
the new repository lookup is covered.
---
Nitpick comments:
In
`@src/main/kotlin/hu/kirdev/schpincer/config/GlobalExceptionControllerAdvice.kt`:
- Line 20: In GlobalExceptionControllerAdvice replace the current error-level
log in the static resource 404 handler (the line using logger.error("No static
resource {} for request '{}'", ex.resourcePath, ex.message)) with a lower level
(e.g., logger.debug or logger.info) so routine missing-resource requests don't
generate error noise; update the call site in the same method to use the chosen
lower log method and keep the same message and placeholders so context
(ex.resourcePath and ex.message) is preserved.
In `@src/main/kotlin/hu/kirdev/schpincer/service/CircleService.kt`:
- Around line 51-54: In CircleService.findByAlias replace the manual empty-check
+ indexed access on results from repo.findAllByAlias(alias) with the idiomatic
Kotlin firstOrNull() (e.g. use repo.findAllByAlias(alias).firstOrNull() or call
firstOrNull() on the results variable) to simplify the code and avoid unsafe
indexing; update the function to return that firstOrNull() result.
In `@src/main/kotlin/hu/kirdev/schpincer/service/UserService.kt`:
- Around line 14-22: The two public methods getById and getByIdOrNull in
UserService are identical; remove getById to avoid confusion and keep
getByIdOrNull as the single source of truth: delete the open fun getById(uid:
String): UserEntity? implementation that calls repo.findByIdOrNull(uid), update
all callers to use getByIdOrNull(uid) instead (or add a small delegating
deprecated wrapper if needed for a transitional period), ensure any
interface/consumer signatures are updated, run tests and compile to catch missed
call sites, and keep repo.findById/findByIdOrNull usage consistent within the
class.
In `@src/main/kotlin/hu/kirdev/schpincer/web/DeployController.kt`:
- Around line 9-13: Move the `@Value-injected` field into the class constructor to
avoid the lateinit var: change DeployController to accept systemAdmins as a
constructor parameter annotated with `@Value`("${schpincer.sysadmins:}") alongside
the existing users: UserService parameter so the class uses constructor
injection for both (replace the private lateinit var systemAdmins: String with a
constructor parameter, keeping the existing users property and annotations).
In `@src/main/kotlin/hu/kirdev/schpincer/web/MainController.kt`:
- Around line 76-84: The branch that handles a missing alias currently returns
the literal view name "error" (see circles.findByAlias(circle) ?: return
"error"), which may not map to a real template and returns a 200 status;
instead, either ensure an error.html template exists or (preferred) replace the
early return with a proper 404 by throwing a
ResponseStatusException(HttpStatus.NOT_FOUND, "Circle not found") (or return a
ResponseEntity with 404) so that the controller method (where
circle.matches(...) and circles.findByAlias(...) live) produces a correct HTTP
404 when the alias is missing.
In `@src/test/kotlin/hu/kirdev/schpincer/service/CancelOrderTest.kt`:
- Around line 36-48: The test stubs orderRepository.findById(...) but the
production call uses the Kotlin extension findByIdOrNull, so update the mock to
target the actual call site: replace the whenever(orderRepository.findById(4))
stub in CancelOrderTest with a stub for orderRepository.findByIdOrNull(4) (or
use org.mockito.kotlin.any() matchers) so that CancelOrderProcedure.loadOrder()
is driven explicitly by the same method it calls; keep the same returned
Optional/nullable value and assertions around procedure.order.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: daeabab1-1aef-4e00-9f1c-f116ec6c0e76
⛔ Files ignored due to path filters (1)
src/main/resources/static/image/stat-bg.svgis excluded by!**/*.svg
📒 Files selected for processing (42)
.mvn/wrapper/maven-wrapper.propertiespom.xmlsrc/main/kotlin/hu/kirdev/schpincer/config/ApplicationConfig.ktsrc/main/kotlin/hu/kirdev/schpincer/config/GlobalExceptionControllerAdvice.ktsrc/main/kotlin/hu/kirdev/schpincer/config/SecurityConfig.ktsrc/main/kotlin/hu/kirdev/schpincer/config/TestingConfig.ktsrc/main/kotlin/hu/kirdev/schpincer/config/WebMvcConfig.ktsrc/main/kotlin/hu/kirdev/schpincer/dto/RoleEntryDto.ktsrc/main/kotlin/hu/kirdev/schpincer/service/CancelOrderProcedure.ktsrc/main/kotlin/hu/kirdev/schpincer/service/ChangeOrderProcedure.ktsrc/main/kotlin/hu/kirdev/schpincer/service/CircleMemberService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/CircleService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/ExtrasService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/ItemPrecedenceService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/ItemService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/MakeOrderProcedure.ktsrc/main/kotlin/hu/kirdev/schpincer/service/OpeningService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/OrderService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/ReviewService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/SchPincerOidcUserService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/StatisticsService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/UserService.ktsrc/main/kotlin/hu/kirdev/schpincer/web/AdminController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/ApiController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/ConfigureController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/DeployController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/FinanceController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/KitchenViewController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/MainController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/ReviewController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/Utility.ktsrc/main/resources/static/css/stats.csssrc/main/resources/static/js/item.jssrc/main/resources/templates/admin.htmlsrc/main/resources/templates/circleRoleList.htmlsrc/main/resources/templates/circleRoleModify.htmlsrc/main/resources/templates/news/news_2023_09_06.htmlsrc/main/resources/templates/stats.htmlsrc/main/resources/templates/userModify.htmlsrc/test/kotlin/hu/kirdev/schpincer/service/CancelOrderTest.ktsrc/test/kotlin/hu/kirdev/schpincer/service/MakeOrderTest.ktsrc/test/kotlin/hu/kirdev/schpincer/service/OrderingServiceTest.kt
💤 Files with no reviewable changes (8)
- src/main/resources/templates/stats.html
- src/test/kotlin/hu/kirdev/schpincer/service/OrderingServiceTest.kt
- src/main/resources/templates/news/news_2023_09_06.html
- src/main/kotlin/hu/kirdev/schpincer/service/StatisticsService.kt
- src/main/kotlin/hu/kirdev/schpincer/web/FinanceController.kt
- src/main/resources/static/css/stats.css
- src/main/kotlin/hu/kirdev/schpincer/config/WebMvcConfig.kt
- src/test/kotlin/hu/kirdev/schpincer/service/MakeOrderTest.kt
Comment on lines
+10
to
20
| open class CircleMemberService(private val repo: CircleMemberRepository) { | ||
|
|
||
| @Transactional(readOnly = false) | ||
| open fun save(member: CircleMemberEntity) { | ||
| repo.save(member) | ||
| } | ||
|
|
||
| @Transactional(readOnly = true) | ||
| open fun getOne(memberId: Long): CircleMemberEntity { | ||
| return repo.getReferenceById(memberId) | ||
| open fun getOne(memberId: Long): CircleMemberEntity? { | ||
| return repo.findByIdOrNull(memberId) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any remaining references to the removed CircleMemberService.getById, and inspect callers of the new getOne for unsafe `!!`.
rg -nP --type=kotlin -C2 '\bcircleMembers?\.(getById|getOne)\b'
rg -nP --type=kotlin -C2 'CircleMemberService[^\n]*\b(getById|getOne)\b'Repository: kir-dev/sch-pincer
Length of output: 44
🏁 Script executed:
# Find all references to CircleMemberService
rg -n "CircleMemberService" --type=kotlin
# Search for any .getOne calls regardless of object name
rg -n "\.getOne\(" --type=kotlin -A1 -B1
# Search for the pattern where result might be unsafely unwrapped
rg -n "\.getOne\([^)]*\)\s*!!" --type=kotlin
# Check if CircleMemberService is imported anywhere
rg -n "import.*CircleMemberService" --type=kotlinRepository: kir-dev/sch-pincer
Length of output: 16628
Fix unsafe unwrapping of nullable getOne() result in CircleMemberService callers.
Migration from getById() to nullable getOne() is complete, but ConfigureController.kt has unsafe !! unwraps that will throw NPE if the member doesn't exist:
- Line 204:
val original = members.getOne(id)!! - Line 242:
val cme = members.getOne(memberId)!!
Use proper null handling instead (e.g., ?: return "redirect:..." or safe navigation with ?.), following the pattern used elsewhere at lines 189 and 230.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/hu/kirdev/schpincer/service/CircleMemberService.kt` around
lines 10 - 20, CircleMemberService.getOne now returns a nullable
CircleMemberEntity but callers in ConfigureController still use unsafe !!
unwrapping; change those calls to handle nulls safely (for example replace val
original = members.getOne(id)!! with val original = members.getOne(id) ?: return
"redirect:..." and similarly replace val cme = members.getOne(memberId)!! with
val cme = members.getOne(memberId) ?: return "redirect:..."), or use safe
navigation (?.) where appropriate to match the null-handling pattern already
used in the controller at other sites.
|
|
||
| getForJsonObject('api/items' + getFilter('') + (profile !== parseInt(0) ? '?circle=' + profile : '')) | ||
| const filter = getFilter(''); | ||
| const url = 'api/items' + filter + (profile !== parseInt(0) ? (filter ? '&' : '?') + 'circle=' + profile : ''); |
There was a problem hiding this comment.
Query delimiter logic generates invalid filtered URLs
When filter is /now or /tomorrow, using (filter ? '&' : '?') creates invalid URLs like api/items/now&circle=... and api/items/now&q=... (missing ? before first query param). This breaks filtered fetches.
Suggested fix
function appendNext(profile = 0) {
@@
- const filter = getFilter('');
- const url = 'api/items' + filter + (profile !== parseInt(0) ? (filter ? '&' : '?') + 'circle=' + profile : '');
+ const filter = getFilter('');
+ const url = new URL('api/items' + filter, window.location.origin);
+ if (profile !== 0) {
+ url.searchParams.set('circle', String(profile));
+ }
- getForJsonObject(url)
+ getForJsonObject(url.pathname.replace(/^\//, '') + url.search)
@@
function searchFor(keyword) {
- const filter = getFilter('');
- const url = 'api/items' + filter + (filter ? '' : '') + (keyword ? (filter ? '&' : '?') + 'q=' + encodeURI(keyword) : '');
+ const filter = getFilter('');
+ const url = new URL('api/items' + filter, window.location.origin);
+ if (keyword) {
+ url.searchParams.set('q', keyword);
+ }
- getForJsonObject(url)
+ getForJsonObject(url.pathname.replace(/^\//, '') + url.search)Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/static/js/item.js` at line 18, The URL builder assembles
query params incorrectly by using (filter ? '&' : '?'), producing strings like
"api/items/now&circle=..."; update the construction for the const url so you
first build the base = 'api/items' + filter and then choose the separator based
on whether the base already contains a '?' (use '&' if it does, otherwise '?')
before appending 'circle=' (only when profile !== 0). Apply the same fix to the
other occurrence referenced (line ~81) so both places use
base.indexOf('?')/includes('?') to decide between '?' and '&' when adding query
parameters.
Filtered circle requests don’t match backend endpoint contract
appendNext() now appends circle even for /api/items/now and /api/items/tomorrow, but backend handlers shown in src/main/kotlin/hu/kirdev/schpincer/web/ApiController.kt:105-122 and :187-204 do not declare @RequestParam circle. So filtered+circle requests won’t be applied as intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/resources/static/js/item.js` at line 18, The frontend is appending
the circle query param for all endpoints (see url construction in appendNext()),
but the ApiController methods that handle /api/items/now and /api/items/tomorrow
do not accept a circle RequestParam; update appendNext() so it only adds
&circle=... when the base endpoint is the paginated /api/items list (i.e., when
filter is empty or refers to the list endpoint) and explicitly skip appending
circle for paths containing "/now" or "/tomorrow" (or alternatively detect
whether the request path equals '/api/items'); locate the url construction in
appendNext() and conditionally concatenate the circle param only for the list
handler.
Gerviba
approved these changes
May 8, 2026
| return listOf() | ||
|
|
||
| val openingIntervalLength = openings.getOne(openingId).intervalLength | ||
| val openingIntervalLength = openings.getOne(openingId)!!.intervalLength |
Member
There was a problem hiding this comment.
openings.getOne(openingId)?.intervalLength ?: return listOf()
maybe, not sure if it is better
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 15 file(s) based on 17 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 15 file(s) based on 17 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/resources/static/js/item.js (1)
20-20: 💤 Low valueSimplify redundant
parseInt(0)comparison.The expression
profile !== parseInt(0)is unnecessarily verbose sinceparseInt(0)always evaluates to0.♻️ Simplified comparison
- const url = base + (profile !== parseInt(0) ? separator + 'circle=' + profile : ''); + const url = base + (profile !== 0 ? separator + 'circle=' + profile : '');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/static/js/item.js` at line 20, The comparison uses parseInt(0) unnecessarily; update the url construction so it compares profile to 0 directly (i.e., in the expression that sets the const url) — replace profile !== parseInt(0) with profile !== 0 (or an equivalent clearer check) in the line that builds url (variables involved: url, profile, base, separator).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/kotlin/hu/kirdev/schpincer/service/SchPincerOidcUserService.kt`:
- Around line 33-35: The comparison uses referential equality (!==) for an enum;
change the check in SchPincerOidcUserService (the if that reads
"existingUser.pekCardType !== card") to use structural equality
("existingUser.pekCardType != card") so CardType enum values are compared
idiomatically before assigning existingUser.pekCardType = card.
- Around line 30-31: The admin membership check uses inconsistent identifiers:
change the check that currently reads existingUser.uid to use
schPincerUser.internalId instead so both admin checks consistently reference the
incoming OIDC identifier; update the block in SchPincerOidcUserService where
admins.contains(existingUser.uid) is evaluated to use
admins.contains(schPincerUser.internalId) and ensure any related comments or
variable assumptions align with schPincerUser.internalId as the canonical admin
identifier.
In `@src/main/kotlin/hu/kirdev/schpincer/web/AdminController.kt`:
- Around line 153-160: In adminDeleteCircleConfirm, verify the circle exists
before deleting related items: call circles.getOne(circleId) first, return the
redirect on null, then call items.deleteByCircle(circleId) and finally
circles.delete(circle); also ensure the sequence (getOne -> deleteByCircle ->
delete) occurs inside the same transaction/context so partial deletes cannot
happen if validation fails.
In `@src/main/kotlin/hu/kirdev/schpincer/web/Utility.kt`:
- Around line 26-27: The current branch in the expression that handles
uploadPath mistakenly prefixes absolute paths with
System.getProperty("user.dir"); swap the branches so that when
uploadPath.startsWith("/") you return uploadPath unchanged, and only prefix
relative paths with System.getProperty("user.dir") + "/" + uploadPath; update
the expression using the uploadPath variable (and the
System.getProperty("user.dir") logic) so absolute paths are treated as absolute.
---
Nitpick comments:
In `@src/main/resources/static/js/item.js`:
- Line 20: The comparison uses parseInt(0) unnecessarily; update the url
construction so it compares profile to 0 directly (i.e., in the expression that
sets the const url) — replace profile !== parseInt(0) with profile !== 0 (or an
equivalent clearer check) in the line that builds url (variables involved: url,
profile, base, separator).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e86aa14a-9ff7-43ae-bdfe-ddc42a3747b5
📒 Files selected for processing (15)
pom.xmlsrc/main/kotlin/hu/kirdev/schpincer/config/SecurityConfig.ktsrc/main/kotlin/hu/kirdev/schpincer/service/CancelOrderProcedure.ktsrc/main/kotlin/hu/kirdev/schpincer/service/ChangeOrderProcedure.ktsrc/main/kotlin/hu/kirdev/schpincer/service/MakeOrderProcedure.ktsrc/main/kotlin/hu/kirdev/schpincer/service/OrderService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/ReviewService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/SchPincerOidcUserService.ktsrc/main/kotlin/hu/kirdev/schpincer/service/UserService.ktsrc/main/kotlin/hu/kirdev/schpincer/web/AdminController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/ApiController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/ConfigureController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/KitchenViewController.ktsrc/main/kotlin/hu/kirdev/schpincer/web/Utility.ktsrc/main/resources/static/js/item.js
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/kotlin/hu/kirdev/schpincer/config/SecurityConfig.kt
- src/main/kotlin/hu/kirdev/schpincer/service/ReviewService.kt
- src/main/kotlin/hu/kirdev/schpincer/service/UserService.kt
- src/main/kotlin/hu/kirdev/schpincer/service/ChangeOrderProcedure.kt
- pom.xml
- src/main/kotlin/hu/kirdev/schpincer/service/OrderService.kt
- src/main/kotlin/hu/kirdev/schpincer/service/MakeOrderProcedure.kt
- src/main/kotlin/hu/kirdev/schpincer/web/ApiController.kt
- src/main/kotlin/hu/kirdev/schpincer/web/ConfigureController.kt
Comment on lines
+30
to
+31
| if (admins.contains(existingUser.uid)) | ||
| existingUser.sysadmin = true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the relationship between uid and internalId in UserEntity and SchPincerOidcUser
# Find UserEntity definition and uid property
ast-grep --pattern 'class UserEntity(
$$$
) {
$$$
}'
# Find SchPincerOidcUser and internalId property
ast-grep --pattern 'class SchPincerOidcUser {
$$$
}'
# Check for any transformations or mappings between uid and internalId
rg -n 'uid|internalId' --type=kotlin -C 2Repository: kir-dev/sch-pincer
Length of output: 20009
Use consistent identifier source for admin check.
Line 30 checks existingUser.uid against the admin list, while Line 52 checks schPincerUser.internalId. Although both should contain the same identifier value, this inconsistent usage reduces clarity and increases maintenance risk. Both checks should reference the same source—preferably schPincerUser.internalId for consistency with the incoming OIDC context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/hu/kirdev/schpincer/service/SchPincerOidcUserService.kt`
around lines 30 - 31, The admin membership check uses inconsistent identifiers:
change the check that currently reads existingUser.uid to use
schPincerUser.internalId instead so both admin checks consistently reference the
incoming OIDC identifier; update the block in SchPincerOidcUserService where
admins.contains(existingUser.uid) is evaluated to use
admins.contains(schPincerUser.internalId) and ensure any related comments or
variable assumptions align with schPincerUser.internalId as the canonical admin
identifier.
Comment on lines
+33
to
35
| if (existingUser.pekCardType !== card) { | ||
| existingUser.pekCardType = card | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the type of pekCardType to confirm correct operator usage
# Find the UserEntity class definition and the pekCardType property
ast-grep --pattern 'class UserEntity {
$$$
}'
# Also check SchPincerOidcUser for cardType property
rg -n 'cardType' --type=kotlin -C 3Repository: kir-dev/sch-pincer
Length of output: 45534
🏁 Script executed:
rg -n "enum class CardType|class CardType|data class CardType" --type=kotlinRepository: kir-dev/sch-pincer
Length of output: 162
Use != instead of !== for enum comparison.
CardType is an enum, and in Kotlin the idiomatic approach for comparing enums is to use != (structural equality), not !== (referential equality). This is also consistent with how CardType is compared throughout the rest of the codebase (see CustomComponentType.kt).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/hu/kirdev/schpincer/service/SchPincerOidcUserService.kt`
around lines 33 - 35, The comparison uses referential equality (!==) for an
enum; change the check in SchPincerOidcUserService (the if that reads
"existingUser.pekCardType !== card") to use structural equality
("existingUser.pekCardType != card") so CardType enum values are compared
idiomatically before assigning existingUser.pekCardType = card.
Comment on lines
153
to
+160
| @PostMapping("/circles/delete/{circleId}/confirm") | ||
| fun adminDeleteCircleConfirm( | ||
| @PathVariable circleId: Long, | ||
| request: HttpServletRequest | ||
| @PathVariable circleId: Long, | ||
| request: HttpServletRequest | ||
| ): String { | ||
| items.deleteByCircle(circleId) | ||
| circles.delete(circles.getOne(circleId)!!) | ||
| val circle = circles.getOne(circleId) ?: return "redirect:/admin/?error=invalidCircleId" | ||
| circles.delete(circle) |
There was a problem hiding this comment.
Validate the circle before deleting related items.
items.deleteByCircle(circleId) executes before you prove circleId is valid. An invalid request can therefore delete item rows and still return invalidCircleId.
Suggested fix
fun adminDeleteCircleConfirm(
`@PathVariable` circleId: Long,
request: HttpServletRequest
): String {
- items.deleteByCircle(circleId)
val circle = circles.getOne(circleId) ?: return "redirect:/admin/?error=invalidCircleId"
+ items.deleteByCircle(circleId)
circles.delete(circle)
return REDIRECT_TO_ADMIN
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @PostMapping("/circles/delete/{circleId}/confirm") | |
| fun adminDeleteCircleConfirm( | |
| @PathVariable circleId: Long, | |
| request: HttpServletRequest | |
| @PathVariable circleId: Long, | |
| request: HttpServletRequest | |
| ): String { | |
| items.deleteByCircle(circleId) | |
| circles.delete(circles.getOne(circleId)!!) | |
| val circle = circles.getOne(circleId) ?: return "redirect:/admin/?error=invalidCircleId" | |
| circles.delete(circle) | |
| `@PostMapping`("/circles/delete/{circleId}/confirm") | |
| fun adminDeleteCircleConfirm( | |
| `@PathVariable` circleId: Long, | |
| request: HttpServletRequest | |
| ): String { | |
| val circle = circles.getOne(circleId) ?: return "redirect:/admin/?error=invalidCircleId" | |
| items.deleteByCircle(circleId) | |
| circles.delete(circle) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/hu/kirdev/schpincer/web/AdminController.kt` around lines 153
- 160, In adminDeleteCircleConfirm, verify the circle exists before deleting
related items: call circles.getOne(circleId) first, return the redirect on null,
then call items.deleteByCircle(circleId) and finally circles.delete(circle);
also ensure the sequence (getOne -> deleteByCircle -> delete) occurs inside the
same transaction/context so partial deletes cannot happen if validation fails.
Comment on lines
+26
to
+27
| if (uploadPath.startsWith("/")) System.getProperty("user.dir") + "/" + uploadPath | ||
| else uploadPath |
There was a problem hiding this comment.
Treat absolute upload paths as absolute.
This branch currently rewrites an absolute path like /var/uploads to ${user.dir}//var/uploads, so uploads land in the wrong directory when app.uploadPath is configured with an absolute path.
Suggested fix
- var path =
- if (uploadPath.startsWith("/")) System.getProperty("user.dir") + "/" + uploadPath
- else uploadPath
+ var path =
+ if (File(uploadPath).isAbsolute) uploadPath
+ else File(System.getProperty("user.dir"), uploadPath).path📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (uploadPath.startsWith("/")) System.getProperty("user.dir") + "/" + uploadPath | |
| else uploadPath | |
| var path = | |
| if (File(uploadPath).isAbsolute) uploadPath | |
| else File(System.getProperty("user.dir"), uploadPath).path |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/hu/kirdev/schpincer/web/Utility.kt` around lines 26 - 27, The
current branch in the expression that handles uploadPath mistakenly prefixes
absolute paths with System.getProperty("user.dir"); swap the branches so that
when uploadPath.startsWith("/") you return uploadPath unchanged, and only prefix
relative paths with System.getProperty("user.dir") + "/" + uploadPath; update
the expression using the uploadPath variable (and the
System.getProperty("user.dir") logic) so absolute paths are treated as absolute.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Refactor