-
Notifications
You must be signed in to change notification settings - Fork 121
Bookings: Show bookings tab when site is eligible #16153
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
|
|
We need to wait for the full site model and update the tabs in observeConditionalTabsAvailabilityWith(_:Site)
RafaelKayumov
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.
Thx for the fix
|
Hey @itsmeichigo. Sry I accidentally put an approval here while looking at the over PR. I'll make a proper review for this one tonight. Don't yet merge. |
RafaelKayumov
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.
LGTM. Works as described. I can see bookings tab on a CIAB site with bookable products. It disappears after switching to a regular site.
We should include a testing step to add bookable product to the CIAB testing store to have the "Bookings" tab presented. I forgot about that requirement myself and couldn't get the tab displayed for a while 😅
Also I noticed that after adding a bookable product on site and refreshing a dashboard + refreshing products - the bookings tab didn't appear. I had to switch sites to get it visible or restart the app. I wonder if such scenario can also happen for users and cause confusion about the feature being broken. WDYT - should we address it or leave as good enough for the MVP?
|
Hey @RafaelKayumov, thanks for the review!
Sorry for missing this step 🤦 I updated the PR description in case someone else comes across this PR in the future.
I discussed with Jorge about this in this thread: p1758087429642489-slack-C03L1NF1EA3. Currently, the tab bar is loaded only once and requires a relaunch or switching stores to update the tab bar for simplicity. Let's discuss there and update the behavior later if needed. I'll wait for @jaclync's review to confirm that the POS works fine with this update before merging. |
| private func hideDotOn(with input: NotificationsBadgeInput) { | ||
| let tag = dotTag(for: input.tab) | ||
| if let subviews = input.tabBar.orderedTabBarActionableViews[input.tabIndex].subviews.first?.subviews { | ||
| if let subviews = input.tabBar.orderedTabBarActionableViews[safe: input.tabIndex]?.subviews.first?.subviews { |
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.
While testing I encountered a crash due to index out of bound here, so I added this for safety.
| isBookingsTabVisible: isBookingsTabVisible) | ||
|
|
||
| // Updates site ID for the POS coordinator to ensure correct data | ||
| posTabCoordinator?.didSwitchStore(id: siteID) |
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'm hoping the siteID can remain constant in POSTabCoordinator. How about reinitializing the POSTabCoordinator here instead of updating the siteID? Would it cause any issues?
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.
@jaclync POSTabCoordinator requires the eligibility checker, which now requires a full site model to check if the site is CIAB. Upon changes of site ID, we don't yet have the full site model so it's not possible to reinitialize the coordinator.
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.
It would be great if the coordinator can be separated from the eligibility checker, but with the current design I don't have a better solution. Please feel free to suggest a solution if you have one in mind, thanks!
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'd consider separating the tab visibility logic in POSEntryPointEligibilityCheckerProtocol to a new protocol that stays in the app, so that POSEntryPointEligibilityCheckerProtocol just has checkEligibility and refreshEligibility that should not depend on the full Site (the CIAB check is performed on the tab visibility).
We're also in the middle of moving POS code into a separate module, and I'm refactoring the dependencies of PointOfSaleEntryPointView passed in POSTabCoordinator. Please go ahead with merging this PR and I'll look into the refactoring as part of the POS module refactoring work.
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.
Thank you!

Part of WOOMOB-1363
Description
This PR adds a new tab for Bookings for CIAB sites with existing bookable products. Due to potential changes in the base branch, there will be updates to where the eligibility check is called, but the rest of the code should be ready for testing.
Testing steps
Testing information
Tested the app with simulator iPhone 17 and iPad Mini iOS 26.
Screenshots
RELEASE-NOTES.txtif necessary.