Bugfix/fixed editing existing order, fixed scroll to existing order #3896
Conversation
witolDark
changed the title
witolDark
requested review from
DenBlaz,
MaxZhylka,
anastasiakuznietsova and
hnativlyubomyr
WalkthroughUpdates adjust order processing and UI flows: introduce conditional payment-cancel-before-redirect in user orders list, add AfterViewInit with requestAnimationFrame scrolling in user orders, integrate existing order details into certificate component with auto-population, map existing bags into forms, and simplify existing-order processing API by removing hasLink logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant OrdersList as UbsUserOrdersListComponent
participant OrderSvc as OrderService
participant Router as Router
participant Snack as MatSnackBarService
User->>OrdersList: Trigger "Continue order"
OrdersList->>OrdersList: Persist orderDetails to localStorage
alt hasPaymentLink is truthy
OrdersList->>OrderSvc: cancelExistingPayment(orderId)
OrderSvc-->>OrdersList: success
OrdersList->>Router: navigate to submit order
else
OrdersList->>Router: navigate to submit order
end
opt cancelExistingPayment fails
OrderSvc-->>OrdersList: error
OrdersList->>Snack: open("Cancellation failed", ...)
end
sequenceDiagram
autonumber
participant Parent as UbsOrderDetailsComponent
participant Cert as UbsOrderCertificateComponent
Parent-->>Parent: existingOrderInfo (truthy)
Parent->>Cert: [orderDetails]=existingOrderInfo
alt orderDetails.certificate has entries
loop for each certificate
Cert->>Cert: addNewCertificate()
Cert->>Cert: set form code
Cert->>Cert: onActivateCertififcate(i)
end
end
sequenceDiagram
autonumber
participant Submit as UBSSubmitOrderComponent
participant OrderSvc as OrderService
participant Backend as API
Submit->>Submit: if existingOrderId >= 0
alt Existing order
Submit->>OrderSvc: processExistingOrder(order, id)
OrderSvc->>Backend: POST /processOrder/{id}
Backend-->>OrderSvc: IProcessOrderResponse
OrderSvc-->>Submit: response
else New order
Submit->>OrderSvc: processNewOrder(order)
OrderSvc->>Backend: POST /processOrder
Backend-->>OrderSvc: response
OrderSvc-->>Submit: response
end
Submit-->>Submit: continue existing redirect flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/ubs/ubs-user/components/ubs-user-orders-list/ubs-user-orders-list.component.ts (1)
239-252: Consider removing the console.error statement.The conditional cancellation flow correctly handles the payment link scenario by canceling the existing payment before redirecting. However, the error handler includes a
console.errorstatement on line 246 which should be removed or replaced with proper logging in production code.Apply this diff to remove the console.error:
.subscribe({ next: () => this.redirectToStepOne(), error: () => { - console.error('Error canceling existing order with ID: ', this.orderId); this.snackBarService.openSnackBar('error'); } });src/app/ubs/ubs-user/components/ubs-user-orders/ubs-user-orders.component.ts (1)
218-234: Consider eliminating the nested setTimeout.The scroll implementation has been improved by using
requestAnimationFrameand clicking the panel header before scrolling. However, there's a nestedsetTimeout(line 227) that might be unnecessary now that you're usingrequestAnimationFrame.Consider this refactor to potentially eliminate the nested timeout:
isPresent.extend = true; - requestAnimationFrame(() => this.scroll(this.orderIdToScroll)); + requestAnimationFrame(() => { + const ord: string = this.orderIdToScroll.toString(); + const element = document.getElementById(ord); + if (element) { + const panel = element.closest('mat-expansion-panel'); + (panel.querySelector('mat-expansion-panel-header') as HTMLElement).click(); + requestAnimationFrame(() => { + panel.scrollIntoView({ + behavior: 'smooth', + block: 'nearest', + inline: 'nearest' + }); + }); + } + }); } scroll(orderId: number): void { - const ord: string = orderId.toString(); - const element = document.getElementById(ord); - if (element) { - const panel = element.closest('mat-expansion-panel'); - (panel.querySelector('mat-expansion-panel-header') as HTMLElement).click(); - setTimeout(() => { - panel.scrollIntoView({ - behavior: 'smooth', - block: 'nearest', - inline: 'nearest' - }); - }, 100); - } + // This method could potentially be removed if the above refactor is applied }This uses a second
requestAnimationFrameinstead ofsetTimeout, which is more performant and ensures the scroll happens after the panel expansion animation begins. However, if the current implementation works well, you can keep it as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/ubs/ubs-user/components/ubs-user-orders-list/ubs-user-orders-list.component.ts(5 hunks)src/app/ubs/ubs-user/components/ubs-user-orders/ubs-user-orders.component.ts(4 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.ts(3 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.html(1 hunks)src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts(3 hunks)src/app/ubs/ubs/components/ubs-submit-order/ubs-submit-order.component.ts(1 hunks)src/app/ubs/ubs/services/order.service.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T22:32:58.734Z
Learnt from: AndrewKozovyi
PR: ita-social-projects/GreenCityClient#3770
File: src/app/ubs/ubs-admin/components/ubs-admin-customers/ubs-admin-customers.component.ts:103-110
Timestamp: 2025-08-01T22:32:58.734Z
Learning: In UbsAdminCustomersComponent (src/app/ubs/ubs-admin/components/ubs-admin-customers/ubs-admin-customers.component.ts), using take(1) instead of takeUntilDestroyed(this.destroyRef) for the adminTableOfCustomersSelector$ subscription prevents excessive additional functionality from being triggered and works correctly for the component's initialization sequence.
Applied to files:
src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.tssrc/app/ubs/ubs-user/components/ubs-user-orders/ubs-user-orders.component.ts
🧬 Code graph analysis (3)
src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts (2)
src/app/ubs/ubs-user/components/ubs-user-orders-list/models/UserOrder.interface.ts (1)
IUserOrderInfo(1-23)src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.ts (1)
orderDetails(34-42)
src/app/ubs/ubs/services/order.service.ts (1)
src/app/ubs/ubs/models/ubs.interface.ts (2)
Order(18-29)IProcessOrderResponse(51-54)
src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.ts (1)
src/app/ubs/ubs-user/components/ubs-user-orders-list/models/UserOrder.interface.ts (1)
IUserOrderInfo(1-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (18.x)
🔇 Additional comments (7)
src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.html (1)
93-93: LGTM!The input binding correctly passes
existingOrderInfoto the certificate component, enabling auto-population of certificate data when editing existing orders.src/app/ubs/ubs/components/ubs-submit-order/ubs-submit-order.component.ts (1)
119-119: LGTM!The removal of the
hasLinkparameter aligns with the simplified API inOrderService.processExistingOrder. The payment cancellation logic has been appropriately moved toubs-user-orders-list.component.tswhere it's handled before redirecting to edit an order.src/app/ubs/ubs/services/order.service.ts (2)
20-20: LGTM!Removing unused RxJS operators (
first,switchMap) improves code cleanliness.
132-133: LGTM!The simplified signature improves the API by removing the
hasLinkparameter and related conditional cancellation logic. The method now has a single responsibility: processing the order. The payment cancellation logic has been appropriately moved to the caller inubs-user-orders-list.component.ts.src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts (1)
296-303: LGTM!The bag mapping logic correctly finds matching bags by
nameEnand updates quantities usingchangeQuantity. The updated formatting with explicit braces improves readability.src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.ts (1)
33-42: LGTM!The input setter correctly auto-populates certificate entries from existing order data. The implementation properly:
- Checks for certificate array existence and length
- Adds new certificate controls
- Sets values from existing data
- Activates each certificate
src/app/ubs/ubs-user/components/ubs-user-orders/ubs-user-orders.component.ts (1)
164-171: LGTM!Moving the scroll-to-order and active couriers initialization to
ngAfterViewInitis correct, as these operations depend on the DOM being ready. This ensures the order element exists before attempting to scroll to it.
| ngOnInit(): void { | ||
| this.store.dispatch(GetUserBonuses()); | ||
|
|
||
| console.log(this.orderDetails); |
There was a problem hiding this comment.
Remove the debug console.log statement.
The console.log statement appears to be debug code left in by mistake. It should be removed before merging.
Apply this diff:
ngOnInit(): void {
this.store.dispatch(GetUserBonuses());
- console.log(this.orderDetails);
this.initForm();📝 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.
| console.log(this.orderDetails); | |
| ngOnInit(): void { | |
| this.store.dispatch(GetUserBonuses()); | |
| this.initForm(); |
🤖 Prompt for AI Agents
In
src/app/ubs/ubs/components/ubs-order-details/ubs-order-certificate/ubs-order-certificate.component.ts
around line 68, remove the debug console.log(this.orderDetails); statement;
delete that line (or replace with an appropriate structured logger call if
needed) and ensure no trailing whitespace remains and tests/linter pass.
| .subscribe(([locations, orderDetails]: [CourierLocations, OrderDetails]) => { | ||
| if (!this.bags || this.bags[0]?.id !== orderDetails.bags[0].id) { | ||
| this.locations = locations; | ||
| console.error(orderDetails); |
There was a problem hiding this comment.
Remove the debug console.error statement.
The console.error statement appears to be debug code left in by mistake. It should be removed before merging.
Apply this diff:
if (!this.bags || this.bags[0]?.id !== orderDetails.bags[0].id) {
this.locations = locations;
- console.error(orderDetails);
this.bags = orderDetails.bags;📝 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.
| console.error(orderDetails); | |
| if (!this.bags || this.bags[0]?.id !== orderDetails.bags[0].id) { | |
| this.locations = locations; | |
| this.bags = orderDetails.bags; |
🤖 Prompt for AI Agents
In src/app/ubs/ubs/components/ubs-order-details/ubs-order-details.component.ts
around line 206, remove the stray debug statement "console.error(orderDetails)"
— simply delete that line so no debug console logging remains in production
code; ensure no other references rely on it and run lint/tests to confirm no
unused variables are introduced.
1 participant
Summary by CodeRabbit