Code Review Bench PR #25593 - fix(api): return original email without OAuth suffix in bookings

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#4

fix(api): return original email without OAuth suffix in bookings

---

Summary by Gitar

• New field:
  • Added displayEmail to BookingAttendee, BookingHost, and ReassignedToDto DTOs for clean email display
• Service enhancement:
  • Implemented getDisplayEmail() method using regex /\+[a-zA-Z0-9]{25}/ to strip OAuth CUID suffixes
• OpenAPI documentation:
  • Updated schema definitions for 6 booking endpoints with displayEmail property
• Test coverage:
  • Added managed-user test suite with 3 tests validating CUID suffix removal

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

⚠️ Bug: Sort removed from getOutputRecurringSeatedBookings

The .sort() call that ordered recurring seated bookings by start time was removed from getOutputRecurringSeatedBookings(), but the sibling method getOutputRecurringBookings() (line 243) still sorts its results by start time. Similarly, getOutputCreateRecurringSeatedBookings() (line 448) also sorts by start. This creates an inconsistency where recurring seated bookings may be returned in a different order depending on which method is called. The sort removal appears unintentional as part of this PR (which is focused on adding displayEmail fields, not changing sort behavior).

_{Was this helpful? React with 👍 / 👎}

Suggested change

	return transformed;
	return transformed.sort((a, b) => new Date(a.start).getTime() - new Date(b.start).getTime());

• Apply suggested fix

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}

[gitar-bot-staging]

Code Review ⚠️ Changes requested 0 resolved / 4 findings

Clean feature implementation adding displayEmail fields, but the sort removal from getOutputRecurringSeatedBookings appears unintentional and could change API response ordering for recurring seated bookings.

⚠️ Bug: Sort removed from getOutputRecurringSeatedBookings

📄 apps/api/v2/src/ee/bookings/2024-08-13/services/output.service.ts:424

The .sort() call that ordered recurring seated bookings by start time was removed from getOutputRecurringSeatedBookings(), but the sibling method getOutputRecurringBookings() (line 243) still sorts its results by start time. Similarly, getOutputCreateRecurringSeatedBookings() (line 448) also sorts by start. This creates an inconsistency where recurring seated bookings may be returned in a different order depending on which method is called. The sort removal appears unintentional as part of this PR (which is focused on adding displayEmail fields, not changing sort behavior).

Suggested fix

    return transformed.sort((a, b) => new Date(a.start).getTime() - new Date(b.start).getTime());

💡 Bug: Lost this context passing getDisplayEmail to .map()

📄 apps/api/v2/src/ee/bookings/2024-08-13/services/output.service.ts:172

In getOutputBooking(), this.getDisplayEmail is passed directly as a .map() callback (line 172), while in getOutputRecurringBooking() (line 317) an arrow function wrapping the call is used instead. Although getDisplayEmail currently doesn't reference this internally (just calls String.replace), passing a class method reference to .map() without binding loses the this context. If getDisplayEmail is ever refactored to use this, this call will break silently. The pattern at line 317 ((guest: string) => this.getDisplayEmail(guest)) is the safer and consistent approach.

Suggested fix

      bookingTransformed.bookingFieldsResponses.displayGuests = bookingTransformed.bookingFieldsResponses.guests.map(
        (guest: string) => this.getDisplayEmail(guest)
      );

💡 Quality: Dead code: BookingAttendee.cleanEmailForDisplay is unused

📄 packages/platform/types/bookings/2024-08-13/outputs/booking.output.ts:60

The static method BookingAttendee.cleanEmailForDisplay() was added to the DTO class but is never called anywhere in the codebase. The actual email cleaning is done by OutputBookingsService_2024_08_13.getDisplayEmail() in the service layer. This duplicated logic in the DTO is dead code that could mislead future developers about where the transformation occurs. Consider removing it to keep the transformation logic centralized in the service layer.

💡 Quality: Pointless getDisplayEmail("unknown") call on literal string

📄 apps/api/v2/src/ee/bookings/2024-08-13/services/output.service.ts:219

In getHost() (line 219), when the user is null, the code sets displayEmail: this.getDisplayEmail("unknown"). Running the regex /\+[a-zA-Z0-9]{25}/ on the literal string "unknown" will always return "unknown" unchanged. This is unnecessary computation and makes the code harder to read. Simply use "unknown" directly, consistent with how the other fallback fields are assigned.

Suggested fix

        displayEmail: "unknown",

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Appended technical summary with 4 categories covering new DTOs, service method, OpenAPI updates, and test suite

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}