-
Notifications
You must be signed in to change notification settings - Fork 5.5k
#19140 - Implement get reservation changes #19145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
WalkthroughAdds two new actions (Get Reservation, List Reservations) and corresponding app-level API methods (getReservation, listReservations refactor, listReservationsInAdministration). Multiple component versions bumped and several action/source modules had their exported Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GetResAction as Get Reservation Action
participant ListResAction as List Reservations Action
participant App as booking_experts.app
participant API as Booking Experts API
User->>GetResAction: call(reservationId)
GetResAction->>App: getReservation(reservationId, $)
App->>API: GET /reservations/{reservationId}
API-->>App: reservation
App-->>GetResAction: response
GetResAction-->>User: summary + data
User->>ListResAction: call(page, perPage [, administrationId])
ListResAction->>App: listReservations({ page, perPage, administrationId, $ })
alt administrationId provided
App->>API: GET /administrations/{administrationId}/reservations?page=&size=
else
App->>API: GET /reservations?page=&size=
end
API-->>App: reservations[]
App-->>ListResAction: response
ListResAction-->>User: "Found N reservations" + data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/booking_experts/actions/get-reservation.js(1 hunks)
🔇 Additional comments (3)
components/booking_experts/actions/get-reservation.js (3)
1-2: LGTM!The import statement is correct and follows Pipedream's standard conventions for making HTTP requests.
3-8: LGTM!The metadata is well-defined with a clear name, description, and appropriate versioning for a new component.
3-30: Verify if "Get Reservations" (list) action is needed.The linked issue #19140 mentions implementing both "Get reservations" (list endpoint) and "Get reservation" (single endpoint) operations. This PR only implements the singular "Get reservation" action. Please confirm whether the list operation should be included in this PR or will be implemented separately.
Reference from PR objectives:
- Get reservations: https://developers.bookingexperts.com/reference/reservations-index
- Get reservation: https://developers.bookingexperts.com/reference/reservations-show
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/booking_experts/actions/list-reservations.js(1 hunks)
🔇 Additional comments (2)
components/booking_experts/actions/list-reservations.js (2)
3-8: Action metadata and key look consistentName, description, key, version, and type follow the usual Pipedream action pattern and look good for a new
list reservationsaction.
23-35: API parameters verified as correct; flagging endpoint mismatch in centralization suggestion for verificationThe web search confirms that
page[number]andAccept: application/vnd.api+jsonare correct per JSON:API specification. However, there is a discrepancy with the suggested refactor:
- The current action calls
GET /v3/reservations(no administrationId scope)- The app's
listReservations()method atbooking_experts.app.mjsrequiresadministrationIdand calls/administrations/{administrationId}/reservations- The action has no
administrationIdpropThe suggested refactor (
this.booking_experts.listReservations($, { page: this.page })) would fail because the method requiresadministrationIdwhich the action doesn't provide. Verify whether:
- The action intentionally calls a different (global) endpoint than the app's scoped method, or
- The action is missing an
administrationIdprop and should be refactored to use it.The
$summarysuggestion is optional (no actions currently use it), andreturn axios(...)would work equivalently toreturn await axios(...)here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/booking_experts/booking_experts.app.mjs (1)
372-379: Add JSDoc documentation for the public API method.The implementation correctly follows the pattern of other administration-scoped methods. Consider adding JSDoc documentation.
Apply this diff:
+ /** + * Lists reservations within a specific administration. + * @param {Object} params - The parameters object + * @param {string} params.administrationId - The ID of the administration + * @param {Object} params.opts - Additional request options including pagination params + * @returns {Promise<Object>} List of reservations for the administration + */ listReservationsInAdministration({ administrationId, ...opts }) {Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/booking_experts/actions/get-reservation/get-reservation.mjs(1 hunks)components/booking_experts/actions/list-reservations/list-reservations.mjs(1 hunks)components/booking_experts/booking_experts.app.mjs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/booking_experts/actions/get-reservation/get-reservation.mjs (1)
components/booking_experts/actions/list-reservations/list-reservations.mjs (1)
response(31-37)
components/booking_experts/actions/list-reservations/list-reservations.mjs (1)
components/booking_experts/actions/get-reservation/get-reservation.mjs (1)
response(24-27)
🔇 Additional comments (2)
components/booking_experts/booking_experts.app.mjs (1)
136-146: LGTM! Clean conditional logic for reservation options.The implementation correctly handles both administration-scoped and organization-wide reservation listing, providing appropriate fallback behavior when no administration context is available.
components/booking_experts/actions/get-reservation/get-reservation.mjs (1)
1-31: LGTM! Well-structured action implementation.The action follows Pipedream conventions correctly:
- Component key uses the proper format
app_name_slug-slugified-component-name- Annotations accurately reflect read-only behavior
- Props leverage propDefinitions as recommended
- Documentation link references official API docs
- Summary message provides clear feedback
components/booking_experts/actions/list-reservations/list-reservations.mjs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/booking_experts/actions/list-bookings/list-bookings.mjs (1)
66-82: Critical: Verified undefined properties cause runtime errors.The verification confirms all three issues in the original review comment are accurate and will cause runtime failures:
- Line 72: References
this.listAdministrationChannels(undefined), should usethis.administrationChannelId- Lines 74-75: Reference
this.createdAtandthis.updatedAt(both undefined)The proposed fixes are correct and should be applied to prevent TypeError exceptions at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
components/booking_experts/actions/add-guest-to-reservation/add-guest-to-reservation.mjs(1 hunks)components/booking_experts/actions/create-agenda-period/create-agenda-period.mjs(1 hunks)components/booking_experts/actions/delete-guest/delete-guest.mjs(1 hunks)components/booking_experts/actions/get-booking/get-booking.mjs(1 hunks)components/booking_experts/actions/get-complex-prices/get-complex-prices.mjs(1 hunks)components/booking_experts/actions/list-availabilities/list-availabilities.mjs(1 hunks)components/booking_experts/actions/list-bookings/list-bookings.mjs(1 hunks)components/booking_experts/actions/list-inventory-objects/list-inventory-objects.mjs(1 hunks)components/booking_experts/actions/list-rentabletype-availabilities/list-rentabletype-availabilities.mjs(1 hunks)components/booking_experts/actions/search-contacts/search-contacts.mjs(1 hunks)components/booking_experts/actions/update-guest/update-guest.mjs(1 hunks)components/booking_experts/sources/booking-updated/booking-updated.mjs(1 hunks)components/booking_experts/sources/inventory-object-updated/inventory-object-updated.mjs(1 hunks)components/booking_experts/sources/new-booking-created/new-booking-created.mjs(1 hunks)components/booking_experts/sources/new-inventory-object-created/new-inventory-object-created.mjs(1 hunks)
🔇 Additional comments (14)
components/booking_experts/actions/get-complex-prices/get-complex-prices.mjs (1)
7-7: Version bump is consistent with PR strategy.The version update from 0.0.3 to 0.0.4 aligns with the broader coordinated release across Booking Experts actions (package 0.2.0 → 0.3.0) that introduces new reservation actions. The action itself is well-structured with no functional changes needed.
components/booking_experts/actions/delete-guest/delete-guest.mjs (1)
7-7: Version bump appears consistent with broader PR versioning strategy.The metadata-only version change from "0.0.1" to "0.0.2" aligns with the PR's coordinated package version increment (0.2.0 → 0.3.0) mentioned in the summary. The action's functional code is clean and follows Pipedream conventions (proper prop definitions, app delegation, summary export).
Verify that this version bump is intentional as part of the package's coordinated release, particularly since the action itself has no functional changes related to the new reservation operations being added in this PR.
components/booking_experts/actions/list-inventory-objects/list-inventory-objects.mjs (1)
7-7: Verify the rationale for the patch version bump.The version has been incremented from 0.0.3 to 0.0.4 with no functional code changes to this file. While the PR context shows a coordinated versioning update across multiple booking_experts components as part of the new reservation feature release, clarify whether this patch bump is intentional (e.g., dependency updates or release coordination) or if it should align differently with the package version change.
components/booking_experts/sources/inventory-object-updated/inventory-object-updated.mjs (1)
8-8: LGTM: Version bump is appropriate.The version bump from 0.0.2 to 0.0.3 aligns with the broader package release (0.2.0 → 0.3.0) and is consistent across all source modules in this PR.
components/booking_experts/sources/new-booking-created/new-booking-created.mjs (1)
8-8: LGTM: Version bump is appropriate.The version bump from 0.0.2 to 0.0.3 is consistent with the package-wide version update.
components/booking_experts/sources/booking-updated/booking-updated.mjs (1)
8-8: LGTM: Version bump is appropriate.The version bump from 0.0.2 to 0.0.3 is consistent with the package-wide version update.
components/booking_experts/sources/new-inventory-object-created/new-inventory-object-created.mjs (1)
8-8: LGTM: Version bump is appropriate.The version bump from 0.0.2 to 0.0.3 is consistent with the package-wide version update.
components/booking_experts/actions/get-booking/get-booking.mjs (1)
7-7: Version bump looks correctThis is a metadata-only update; the action logic remains unchanged and the new version aligns with the broader Booking Experts release in this PR.
components/booking_experts/actions/search-contacts/search-contacts.mjs (1)
8-8: Search Contacts action version bump is fineOnly the
versionfield changed; existing validation and error handling remain intact, so this is a safe metadata update.components/booking_experts/actions/list-availabilities/list-availabilities.mjs (1)
7-7: List Availabilities version increment is appropriateThis is a straightforward metadata-only version increment with no behavioral changes to the action.
components/booking_experts/actions/list-rentabletype-availabilities/list-rentabletype-availabilities.mjs (1)
8-8: RentableType availabilities action version bump is consistentOnly the
versionfield changed; the existing validation around start/end dates and API call structure is preserved.components/booking_experts/actions/add-guest-to-reservation/add-guest-to-reservation.mjs (1)
7-7: Add Guest to Reservation version bump is safeThe update only touches the
versionmetadata; the request payload and action behavior remain the same.components/booking_experts/actions/update-guest/update-guest.mjs (1)
7-7: Update Guest action version increment looks goodThis is a metadata-only version change; existing props and update logic are unaffected.
components/booking_experts/actions/list-bookings/list-bookings.mjs (1)
7-7: LGTM: Version bump is appropriate.The version increment from "0.0.3" to "0.0.4" aligns with the broader package version update in this PR.
components/booking_experts/actions/create-agenda-period/create-agenda-period.mjs
Show resolved
Hide resolved
michelle0927
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for QA!
|
Hi everyone, all test cases are passed! Ready for release! Test reports
|
Implements a new Booking Experts action to fetch reservations by ID, following API documentation.
Closes #19140.
Please let me know if you’d like any changes.
Summary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.