- 
                Notifications
    You must be signed in to change notification settings 
- Fork 119
[Bookings] Update attendance status remotely #16280
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
[Bookings] Update attendance status remotely #16280
Conversation
| 
 
 | 
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.
I tested the PR and confirmed that it works as described. I left some nit-picking in the comments but pre-approving.
| dateCreated = booking.dateCreated ?? dateCreated | ||
| dateModified = booking.dateModified ?? dateModified | 
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.
❓ Are you sure we want to keep the old dates here? If so, please leave a comment in the code for why that's necessary.
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.
Added in b3991c7
| return onCompletion(UpdateBookingStatusError.undefinedState) | ||
| } | ||
|  | ||
| Task { | 
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.
❓ Is it necessary to trigger the completion closure on the main thread with @MainActor 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.
Added in 78de80e
| footerText: Localization.attendanceSectionFooterText, | ||
| content: .attendance(attendanceContent) | ||
| ), | ||
| at: 3 | 
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.
❓ Is it safe to use this index if the customer section is absent? Should we trigger setupSections upon changes of section visibility in updateDisplayProperties to avoid having to work with indices?
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.
I prefer keep the static sections without recreation there. I added section index calculation in 8ed229e
In case if design change and we get more dynamic sections - we can revisit this and start rebuilding sections for each booking update
| ) { error in | ||
| if let error { | ||
| DDLogError("⛔️ Error updating booking attendance status: \(error)") | ||
| // TODO: Show an error notice to the user | 
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.
Should we show a notice here to let the user know about the failure?
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.
I'll handle this in a separate PR
# Conflicts: # Modules/Tests/NetworkingTests/Remote/BookingsRemoteTests.swift # Modules/Tests/YosemiteTests/Mocks/MockBookingsRemote.swift # Modules/Tests/YosemiteTests/Stores/BookingStoreTests.swift

WOOMOB-1532
Description
dateCreatedanddateModifiedoptional inYosemite.Bookingto ignorenilvalues returned inPUTmethod.BookingRemote.updateBookingmethod to update booking attendance status remotely.Testing steps
/wc-bookings/v2/bookings/{booking-id}&_method=putin Proxyman. Make sure the status is rolled back shortly after switching.Demo
Booking-status-update.mov
RELEASE-NOTES.txtif necessary.