feat(pos-integration): DOMA-12621 inject pos integration to condo via b2bapp config #7110
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds POS integration end-to-end: DB schema, migration, GraphQL types/queries, access controls, server/test utilities, client IFrame alert and hooks, last-testing-receipt fetcher, table renderer/column, and UI flow to surface an alert/spinner, fetch/refetch the last testing receipt, and sort/highlight the matching payment row. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as rgba(66,133,244,0.5)
participant PosAlertHook as rgba(34,197,94,0.5)
participant GQL as rgba(234,88,12,0.5)
participant ReceiptHook as rgba(168,85,247,0.5)
participant Table as rgba(14,165,233,0.5)
UI->>PosAlertHook: mount (orgId)
PosAlertHook->>GQL: query getB2BAppContextWithPosIntegrationConfig(orgId)
GQL-->>PosAlertHook: return posIntegrationConfig URLs
PosAlertHook->>UI: render PosIntegrationAlert (IFrame) + loading=true
Note over PosAlertHook,UI: IFrame loads → onLoad fires
PosAlertHook->>UI: set loading=false
UI->>ReceiptHook: mount (skipUntilAuthenticated = alertLoading)
ReceiptHook->>GQL: GET fetchLastPosReceiptUrl (credentials)
GQL-->>ReceiptHook: return lastTestingPosReceipt|null
ReceiptHook->>UI: provide lastTestingPosReceipt + loading
UI->>Table: render columns (include posReceiptUrl), use sortedDataSource and highlight row
Table->>UI: user interacts (Download / Tooltip)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cbfa38b59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| status: B2BAppContextStatusType.Finished, | ||
| organization: { id: organization.id }, | ||
| app: { posIntegrationConfig_is_null: false, deletedAt: null }, |
There was a problem hiding this comment.
On initial render, useOrganization() can return organization as undefined (e.g., while the org context is still loading or when the user has no org). This hook immediately dereferences organization.id inside the where clause, which will throw before the skip option is evaluated. The result is a runtime crash and the payments page never renders in those states. Consider guarding the access (e.g., organization?.id) or returning early until the org is available.
Useful? React with 👍 / 👎.
| where: { | ||
| status: B2BAppContextStatusType.Finished, | ||
| organization: { id: organization.id }, |
There was a problem hiding this comment.
This hook has the same unsafe access pattern as the alert hook: it uses organization.id inside the query where before the skip flag can short‑circuit. If organization is still undefined during mount, the hook throws and prevents the payments table from rendering. Use organization?.id in the query or defer the query until the org is present.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx`:
- Around line 153-157: Remove the redundant useEffect that immediately calls
refetchLastTestingPosReceipt when areAlertLoading flips to false;
MarketplacePaymentsTableContent already mounts after areAlertLoading is false
and triggers the initial fetch, so delete the effect block referencing
useEffect, areAlertLoading and refetchLastTestingPosReceipt to avoid the
duplicate refetch on mount.
In `@apps/condo/domains/miniapp/schema/B2BAppPosIntegrationConfig.test.js`:
- Around line 119-121: The test currently expects an AccessDeniedError when an
anonymous user calls B2BAppPosIntegrationConfig.delete; change the assertion to
expect an AuthenticationError instead by replacing the
expectToThrowAccessDeniedErrorToObj usage with the corresponding
expectToThrowAuthenticationErrorToObj helper for the anonymous,
createdB2BAppPosIntegrationConfig.id deletion call (keep the same anonymous
variable and B2BAppPosIntegrationConfig.delete invocation).
In `@apps/condo/lang/es/es.json`:
- Around line 2427-2430: Update the Spanish strings to use informal "tú" tone:
change pages.condo.payments.posReceiptVerifyTitle from "Verifique la exactitud
del recibo" to an informal variant (e.g., "Verifica la exactitud del recibo")
and change pages.condo.payments.posReceiptVerifyDescription from the formal
phrasing to informal (e.g., "Si todos los datos son correctos, puedes activar la
fiscalización. Si hay errores, contacta al soporte técnico.") so the tone
matches other UI strings.
♻️ Duplicate comments (2)
apps/condo/domains/acquiring/hooks/usePosIntegrationAlert.tsx (1)
21-28: Guard against undefined organization before building thewherefilter.On initial render,
useOrganization()can yieldorganizationas undefined, soorganization.idthrows beforeskipis evaluated (Line 24). This matches the prior review finding.🛠️ Proposed fix
- organization: { id: organization.id }, + organization: { id: organization?.id },apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts (1)
34-40: Guardorganizationbefore dereferencing in querywhereclause.The query's
whereclause accessesorganization.iddirectly (line 36), but theskipoption on line 40 uses optional chaining (organization?.id). Iforganizationisundefinedduring mount, the where clause evaluation may throw before the skip condition can prevent the query. Use optional chaining consistently:🐛 Proposed fix
} = B2BAppContext.useObjects({ where: { status: B2BAppContextStatusType.Finished, - organization: { id: organization.id }, + organization: { id: organization?.id }, app: { posIntegrationConfig_is_null: false, deletedAt: null }, deletedAt: null, }, }, { skip: !organization?.id })
🧹 Nitpick comments (5)
apps/condo/domains/acquiring/components/payments/getPosReceiptUrlRender.tsx (1)
9-14: Preferinterfaceovertypefor object shape definitions.Per coding guidelines, use
interfacefor defining object shapes in TypeScript.♻️ Suggested refactor
-type GetPosReceiptUrlRenderParams = { +interface GetPosReceiptUrlRenderParams { linkText: string verifyTitle: string verifyDescription: string lastTestingPosReceipt?: LastTestingPosReceiptData }apps/condo/domains/acquiring/hooks/usePaymentsTableColumns.ts (1)
20-22: Preferinterfaceovertypefor object shape definitions.Per coding guidelines, use
interfacefor defining object shapes in TypeScript.♻️ Suggested refactor
-type PaymentsTableColumnsOptions = { +interface PaymentsTableColumnsOptions { lastTestingPosReceipt?: LastTestingPosReceiptData }apps/condo/domains/marketplace/hooks/useMarketplacePaymentTableColumns.tsx (1)
20-22: Preferinterfaceovertypefor object shape definitions.Per coding guidelines, use
interfacefor defining object shapes in TypeScript.♻️ Suggested refactor
-type MarketplacePaymentsTableColumnsOptions = { +interface MarketplacePaymentsTableColumnsOptions { lastTestingPosReceipt?: LastTestingPosReceiptData }apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx (1)
297-301: Minor: Consider returning an empty object for consistency.The
onRowcallback implicitly returnsundefinedwhen the condition doesn't match. While this works, returning an empty object explicitly improves readability and consistency:♻️ Optional refactor
onRow={(record) => { if (lastTestingPosReceipt && lastTestingPosReceipt?.condoPaymentId === record.id) { return { style: { backgroundColor: colors.orange[1] } } } + return {} }}apps/condo/domains/miniapp/schema/B2BAppPosIntegrationConfig.js (1)
11-12: Consider a more descriptive schemaDoc.The current
schemaDocsimply repeats the schema name. A more descriptive documentation would help developers understand the purpose of this configuration entity.Suggested improvement
const B2BAppPosIntegrationConfig = new GQLListSchema('B2BAppPosIntegrationConfig', { - schemaDoc: 'B2BAppPosIntegrationConfig', + schemaDoc: 'Configuration for B2B App POS (Point of Sale) integration, containing URLs for payment alerts and receipt fetching.', fields: {
| useEffect(() => { | ||
| if (!areAlertLoading) { | ||
| refetchLastTestingPosReceipt() | ||
| } | ||
| }, [areAlertLoading, refetchLastTestingPosReceipt]) |
There was a problem hiding this comment.
Avoid the immediate duplicate refetch.
MarketplacePaymentsTableContent only mounts once areAlertLoading is false, so the hook already fetches on mount. This effect triggers a second, immediate refetch every time, which is redundant.
✅ Simplest fix: remove the effect
- useEffect(() => {
- if (!areAlertLoading) {
- refetchLastTestingPosReceipt()
- }
- }, [areAlertLoading, refetchLastTestingPosReceipt])📝 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.
| useEffect(() => { | |
| if (!areAlertLoading) { | |
| refetchLastTestingPosReceipt() | |
| } | |
| }, [areAlertLoading, refetchLastTestingPosReceipt]) |
🤖 Prompt for AI Agents
In `@apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx`
around lines 153 - 157, Remove the redundant useEffect that immediately calls
refetchLastTestingPosReceipt when areAlertLoading flips to false;
MarketplacePaymentsTableContent already mounts after areAlertLoading is false
and triggers the initial fetch, so delete the effect block referencing
useEffect, areAlertLoading and refetchLastTestingPosReceipt to avoid the
duplicate refetch on mount.
| await expectToThrowAccessDeniedErrorToObj(async () => { | ||
| await B2BAppPosIntegrationConfig.delete(anonymous, createdB2BAppPosIntegrationConfig.id) | ||
| }) |
There was a problem hiding this comment.
Anonymous user should throw AuthenticationError, not AccessDeniedError.
For consistency with other tests in this file (lines 57-61, 95-99, 165-170), anonymous users should receive an AuthenticationError rather than AccessDeniedError. The current expectation may pass if the access control returns AccessDenied before checking authentication, but this is inconsistent with the pattern used elsewhere in the test file.
Proposed fix
await expectToThrowAccessDeniedErrorToObj(async () => {
await B2BAppPosIntegrationConfig.delete(user, createdB2BAppPosIntegrationConfig.id)
})
- await expectToThrowAccessDeniedErrorToObj(async () => {
+ await expectToThrowAuthenticationErrorToObj(async () => {
await B2BAppPosIntegrationConfig.delete(anonymous, createdB2BAppPosIntegrationConfig.id)
})🤖 Prompt for AI Agents
In `@apps/condo/domains/miniapp/schema/B2BAppPosIntegrationConfig.test.js` around
lines 119 - 121, The test currently expects an AccessDeniedError when an
anonymous user calls B2BAppPosIntegrationConfig.delete; change the assertion to
expect an AuthenticationError instead by replacing the
expectToThrowAccessDeniedErrorToObj usage with the corresponding
expectToThrowAuthenticationErrorToObj helper for the anonymous,
createdB2BAppPosIntegrationConfig.id deletion call (keep the same anonymous
variable and B2BAppPosIntegrationConfig.delete invocation).
| "pages.condo.payments.posReceiptColumn": "Recibo", | ||
| "pages.condo.payments.posReceiptLink": "Ver", | ||
| "pages.condo.payments.posReceiptVerifyTitle": "Verifique la exactitud del recibo", | ||
| "pages.condo.payments.posReceiptVerifyDescription": "Si todos los datos son correctos, puede activar la fiscalización. Si hay errores, contacte al soporte técnico.", |
There was a problem hiding this comment.
Align the tone with the rest of the Spanish UI (tú vs. usted).
These strings use formal address (“Verifique”, “puede”, “contacte”), while most of the UI uses informal “tú” (e.g., “Introduce…”, “No tienes…”). Consider switching to informal for consistency.
✏️ Proposed text update
-"pages.condo.payments.posReceiptVerifyTitle": "Verifique la exactitud del recibo",
-"pages.condo.payments.posReceiptVerifyDescription": "Si todos los datos son correctos, puede activar la fiscalización. Si hay errores, contacte al soporte técnico.",
+"pages.condo.payments.posReceiptVerifyTitle": "Verifica la exactitud del recibo",
+"pages.condo.payments.posReceiptVerifyDescription": "Si todos los datos son correctos, puedes activar la fiscalización. Si hay errores, contacta al soporte técnico.",📝 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.
| "pages.condo.payments.posReceiptColumn": "Recibo", | |
| "pages.condo.payments.posReceiptLink": "Ver", | |
| "pages.condo.payments.posReceiptVerifyTitle": "Verifique la exactitud del recibo", | |
| "pages.condo.payments.posReceiptVerifyDescription": "Si todos los datos son correctos, puede activar la fiscalización. Si hay errores, contacte al soporte técnico.", | |
| "pages.condo.payments.posReceiptColumn": "Recibo", | |
| "pages.condo.payments.posReceiptLink": "Ver", | |
| "pages.condo.payments.posReceiptVerifyTitle": "Verifica la exactitud del recibo", | |
| "pages.condo.payments.posReceiptVerifyDescription": "Si todos los datos son correctos, puedes activar la fiscalización. Si hay errores, contacta al soporte técnico.", |
🤖 Prompt for AI Agents
In `@apps/condo/lang/es/es.json` around lines 2427 - 2430, Update the Spanish
strings to use informal "tú" tone: change
pages.condo.payments.posReceiptVerifyTitle from "Verifique la exactitud del
recibo" to an informal variant (e.g., "Verifica la exactitud del recibo") and
change pages.condo.payments.posReceiptVerifyDescription from the formal phrasing
to informal (e.g., "Si todos los datos son correctos, puedes activar la
fiscalización. Si hay errores, contacta al soporte técnico.") so the tone
matches other UI strings.
AleX83Xpert
force-pushed
the
feat/pos-integration/doma-12621/inject-pos-integration-to-condo
branch
from
fc2ee1d to
81c1bf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx`:
- Around line 129-133: The useEffect in PaymentsTable.tsx that depends on
areAlertLoading and refetchLastTestingPosReceipt causes a redundant immediate
refetch because PaymentsTableContent already mounts after areAlertLoading is
false and fetches on mount; remove this useEffect block (the effect that calls
refetchLastTestingPosReceipt when !areAlertLoading) to avoid duplicate
refetches, keeping the initial mount fetch in PaymentsTableContent intact and
leaving refetchLastTestingPosReceipt usage elsewhere unchanged.
♻️ Duplicate comments (2)
apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts (1)
33-40: Guardorganizationbefore dereferencing in querywhere.
The query usesorganization.idbefore theskipcan take effect, which can throw on initial render whenorganizationis undefined. This was already flagged previously.apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx (1)
153-157: Avoid the immediate duplicate refetch.
MarketplacePaymentsTableContentonly mounts onceareAlertLoadingis false, so the hook already fetches on mount. This effect triggers a second, immediate refetch every time, which is redundant.✅ Simplest fix: remove the effect
- useEffect(() => { - if (!areAlertLoading) { - refetchLastTestingPosReceipt() - } - }, [areAlertLoading, refetchLastTestingPosReceipt])
🧹 Nitpick comments (7)
apps/condo/domains/miniapp/components/IFrame.tsx (1)
31-40: Use an interface for props shape.Repo guidelines prefer
interfaceovertypefor object-shaped props in TS/TSX. Please switchIFramePropsto an interface.♻️ Proposed change
-export type IFrameProps = { +export interface IFrameProps { src: string hidden?: boolean reloadScope: 'user' | 'organization' | null withLoader?: boolean withPrefetch?: boolean withResize?: boolean allowFullscreen?: boolean onLoad?: () => void -} +}As per coding guidelines, please prefer
interfacefor object shapes.apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts (1)
57-67: Use structured logger instead ofconsole.error.Per repo logging guidelines, prefer
getLogger()with structured fields for error reporting.♻️ Suggested update
+import { getLogger } from '@open-condo/keystone/logging' + +const logger = getLogger('usePosIntegrationLastTestingPosReceipt') @@ - if (!response.ok) { - console.error('Failed to fetch last receipt:', response.statusText) + if (!response.ok) { + logger.error({ + msg: 'Failed to fetch last receipt', + status: response.status, + data: { statusText: response.statusText }, + }) setLastTestingPosReceipt(null) return } @@ - } catch (error) { - console.error('Error fetching last receipt:', error) + } catch (error) { + logger.error({ msg: 'Error fetching last receipt', err: error }) setLastTestingPosReceipt(null) } finally { setLoading(false) }As per coding guidelines, prefer structured logging via
getLogger().apps/condo/domains/acquiring/hooks/usePaymentsTableColumns.ts (1)
20-22: Useinterfaceinstead oftypefor object shape.Project TS guidelines prefer
interfacefor object shapes.♻️ Suggested update
-type PaymentsTableColumnsOptions = { +interface PaymentsTableColumnsOptions { lastTestingPosReceipt?: LastTestingPosReceiptData }As per coding guidelines, prefer
interfacefor object shapes in TS.apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx (1)
297-301: Minor: Clean up conditional logic and return value.Two small improvements:
- The optional chaining
lastTestingPosReceipt?.condoPaymentIdis redundant after the truthy checklastTestingPosReceipt &&.onRowimplicitly returnsundefinedwhen the condition is false; explicitly returning an empty object or{}is clearer and safer for potential future table implementations.✨ Suggested improvement
onRow={(record) => { - if (lastTestingPosReceipt && lastTestingPosReceipt?.condoPaymentId === record.id) { + if (lastTestingPosReceipt && lastTestingPosReceipt.condoPaymentId === record.id) { return { style: { backgroundColor: colors.orange[1] } } } + return {} }}apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx (3)
112-114: Consider using a type alias instead of interface.Based on learnings, prefer type aliases over interfaces for object shapes in this repository.
✨ Suggested change
-interface MarketplacePaymentsTableContentProps { - areAlertLoading: boolean -} +type MarketplacePaymentsTableContentProps = { + areAlertLoading: boolean +}
361-365: Minor: Clean up redundant optional chaining.Same pattern as in
PaymentsTable.tsx: the optional chaining is redundant after the truthy check, and returning an empty object explicitly is cleaner.✨ Suggested improvement
onRow={(record) => { - if (lastTestingPosReceipt && lastTestingPosReceipt?.condoPaymentId === record.id) { + if (lastTestingPosReceipt && lastTestingPosReceipt.condoPaymentId === record.id) { return { style: { backgroundColor: colors.orange[1] } } } + return {} }}
172-177: Missing dependency in useMemo.
searchPaymentsQuerydepends oninvoiceOrganizationQuerybut the dependency array only includesorgIdandshowPaymentsOnlyInDoneStatus. While this works becauseinvoiceOrganizationQueryis memoized withorgId, explicitly including it would be more maintainable.✨ Suggested fix
- }, [orgId, showPaymentsOnlyInDoneStatus]) + }, [invoiceOrganizationQuery, showPaymentsOnlyInDoneStatus])
| useEffect(() => { | ||
| if (!areAlertLoading) { | ||
| refetchLastTestingPosReceipt() | ||
| } | ||
| }, [areAlertLoading, refetchLastTestingPosReceipt]) |
There was a problem hiding this comment.
Avoid the immediate duplicate refetch.
PaymentsTableContent only mounts once areAlertLoading is false, so the hook already fetches on mount. This effect triggers a second, immediate refetch every time, which is redundant.
✅ Simplest fix: remove the effect
- useEffect(() => {
- if (!areAlertLoading) {
- refetchLastTestingPosReceipt()
- }
- }, [areAlertLoading, refetchLastTestingPosReceipt])📝 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.
| useEffect(() => { | |
| if (!areAlertLoading) { | |
| refetchLastTestingPosReceipt() | |
| } | |
| }, [areAlertLoading, refetchLastTestingPosReceipt]) |
🤖 Prompt for AI Agents
In `@apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx` around
lines 129 - 133, The useEffect in PaymentsTable.tsx that depends on
areAlertLoading and refetchLastTestingPosReceipt causes a redundant immediate
refetch because PaymentsTableContent already mounts after areAlertLoading is
false and fetches on mount; remove this useEffect block (the effect that calls
refetchLastTestingPosReceipt when !areAlertLoading) to avoid duplicate
refetches, keeping the initial mount fetch in PaymentsTableContent intact and
leaving refetchLastTestingPosReceipt usage elsewhere unchanged.
81c1bf9 to
0fcaca9
Compare
AleX83Xpert
added
the
✋🙂 Review please
toplenboren
left a comment
toplenboren
left a comment
There was a problem hiding this comment.
Loooks ok, but note that you are downgrading next here
yarn.lock
Outdated
| lodash: ^4.17.21 | ||
| mkdirp: ^3.0.1 | ||
| next: ^15.5.7 | ||
| next: ^15.4.6 |
There was a problem hiding this comment.
Hm, I'll check it. Thanks!
There was a problem hiding this comment.
Fixed. I've set the version to the same as the condo.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts`:
- Around line 57-67: In usePosIntegrationLastTestingPosReceipt, replace the two
console.error calls with the repo's structured logger: call
getLogger().error(...) instead and include structured fields (e.g., { error,
status: response?.status, statusText: response?.statusText }) so the failure
branch (when !response.ok) logs response.status and response.statusText
alongside a descriptive message and setLastTestingPosReceipt(null), and the
catch branch logs the thrown error object as a field (e.g., { error }) with a
clear message before calling setLastTestingPosReceipt(null); locate these
changes around the response handling and the catch in
usePosIntegrationLastTestingPosReceipt.
In `@apps/condo/domains/miniapp/access/B2BAppPosIntegrationConfig.js`:
- Around line 10-15: canReadB2BAppPosIntegrationConfigs currently returns {}
which grants all authenticated users global read access; restrict it to the same
scope as the manage rule. Change canReadB2BAppPosIntegrationConfigs to mirror
canManageB2BAppPosIntegrationConfigs (or the org-scoping logic you use
elsewhere): return true/{} only for global admins, otherwise return a filter
object that limits results to the user's organisation (e.g. return {
organization: { id: user.organizationId } }) or return false if the user lacks
that org relationship; ensure you reuse the same checks/utility used by
canManageB2BAppPosIntegrationConfigs to avoid divergence.
♻️ Duplicate comments (3)
apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts (1)
33-40: Guard organization before dereference in the query filter.Line 36 accesses
organization.idbefore theskipguard can short‑circuit, which can throw during initial render.🐛 Suggested fix
- organization: { id: organization.id }, + organization: { id: organization?.id },apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx (1)
129-133: Avoid the immediate duplicate refetch.
PaymentsTableContentonly mounts onceareAlertLoadingis false, so the hook already fetches on mount. This effect triggers a second, immediate refetch every time, which is redundant.✅ Simplest fix: remove the effect
- useEffect(() => { - if (!areAlertLoading) { - refetchLastTestingPosReceipt() - } - }, [areAlertLoading, refetchLastTestingPosReceipt])apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx (1)
153-157: Remove the redundant refetch effect.
MarketplacePaymentsTableContentmounts only afterareAlertLoadingis false, and the hook already fetches on mount. This effect triggers a second, immediate fetch.✅ Simplest fix
- useEffect(() => { - if (!areAlertLoading) { - refetchLastTestingPosReceipt() - } - }, [areAlertLoading, refetchLastTestingPosReceipt])
🧹 Nitpick comments (3)
apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx (1)
297-301: Return empty object when condition is false to avoidundefined.The
onRowcallback returnsundefinedwhen the condition doesn't match. While this may work, it's inconsistent and can cause subtle issues with type expectations.🔧 Suggested fix
onRow={(record) => { if (lastTestingPosReceipt && lastTestingPosReceipt?.condoPaymentId === record.id) { return { style: { backgroundColor: colors.orange[1] } } } + return {} }}apps/condo/domains/miniapp/schema/B2BAppPosIntegrationConfig.js (1)
15-25: Consider adding URL validation for the URL fields.Both
paymentsAlertPageUrlandfetchLastPosReceiptUrlare plainTextfields accepting any string. Since these are used for iframe sources and fetch requests respectively (as documented in the schema), consider adding avalidateInputhook to ensure valid URLs, following the validation patterns used in similar URL fields throughout the codebase (e.g.,DEVELOPER_URL_FIELD,IFRAME_URL_FIELDin B2BApp.js).apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx (1)
4-4: Prefer@open-condo/uifor Space/Spin (if available).These new UI elements should come from
@open-condo/uito stay consistent with the UI layer. If@open-condo/uiexportsSpaceandSpin, consider switching the imports. As per coding guidelines, prefer@open-condo/uicomponents for GUI elements.♻️ Suggested adjustment (if supported by `@open-condo/ui`)
-import { Col, Row, RowProps, Space, Spin } from 'antd' +import { Col, Row, RowProps } from 'antd' ... -import { Button, Modal, Checkbox, Typography } from '@open-condo/ui' +import { Button, Modal, Checkbox, Space, Spin, Typography } from '@open-condo/ui'Also applies to: 97-105
| if (!response.ok) { | ||
| console.error('Failed to fetch last receipt:', response.statusText) | ||
| setLastTestingPosReceipt(null) | ||
| return | ||
| } | ||
|
|
||
| const data: LastTestingPosReceiptData | null = await response.json() | ||
| setLastTestingPosReceipt(data) | ||
| } catch (error) { | ||
| console.error('Error fetching last receipt:', error) | ||
| setLastTestingPosReceipt(null) |
There was a problem hiding this comment.
Replace console.error with structured logger.
This repo’s JS/TS logging guideline requires getLogger() and structured fields.
🧭 Suggested fix
import { useCallback, useEffect, useState } from 'react'
+import { getLogger } from '@open-condo/keystone/logging'
import { useOrganization } from '@open-condo/next/organization'
import { B2BAppContext } from '@condo/domains/miniapp/utils/clientSchema'
+const logger = getLogger('usePosIntegrationLastTestingPosReceipt')
+
@@
- if (!response.ok) {
- console.error('Failed to fetch last receipt:', response.statusText)
+ if (!response.ok) {
+ logger.error({ msg: 'Failed to fetch last receipt', data: { status: response.status, statusText: response.statusText } })
setLastTestingPosReceipt(null)
return
}
@@
- } catch (error) {
- console.error('Error fetching last receipt:', error)
+ } catch (error) {
+ logger.error({ msg: 'Error fetching last receipt', err: error })
setLastTestingPosReceipt(null)
} finally {🤖 Prompt for AI Agents
In `@apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts`
around lines 57 - 67, In usePosIntegrationLastTestingPosReceipt, replace the two
console.error calls with the repo's structured logger: call
getLogger().error(...) instead and include structured fields (e.g., { error,
status: response?.status, statusText: response?.statusText }) so the failure
branch (when !response.ok) logs response.status and response.statusText
alongside a descriptive message and setLastTestingPosReceipt(null), and the
catch branch logs the thrown error object as a field (e.g., { error }) with a
clear message before calling setLastTestingPosReceipt(null); locate these
changes around the response handling and the catch in
usePosIntegrationLastTestingPosReceipt.
| async function canReadB2BAppPosIntegrationConfigs ({ authentication: { item: user } }) { | ||
| if (!user) return throwAuthenticationError() | ||
| if (user.deletedAt) return false | ||
|
|
||
| return {} | ||
| } |
There was a problem hiding this comment.
Scope read access; current rule exposes all configs to any authenticated user.
If global read isn’t intended, align read access with the manage rule to avoid cross‑org leakage of integration URLs.
🔐 Suggested adjustment
-async function canReadB2BAppPosIntegrationConfigs ({ authentication: { item: user } }) {
+async function canReadB2BAppPosIntegrationConfigs ({ authentication: { item: user }, listKey, originalInput, operation }) {
if (!user) return throwAuthenticationError()
if (user.deletedAt) return false
-
- return {}
+ if (user.isSupport || user.isAdmin) return true
+ return await canDirectlyManageSchemaObjects(user, listKey, originalInput, operation)
}🤖 Prompt for AI Agents
In `@apps/condo/domains/miniapp/access/B2BAppPosIntegrationConfig.js` around lines
10 - 15, canReadB2BAppPosIntegrationConfigs currently returns {} which grants
all authenticated users global read access; restrict it to the same scope as the
manage rule. Change canReadB2BAppPosIntegrationConfigs to mirror
canManageB2BAppPosIntegrationConfigs (or the org-scoping logic you use
elsewhere): return true/{} only for global admins, otherwise return a filter
object that limits results to the user's organisation (e.g. return {
organization: { id: user.organizationId } }) or return false if the user lacks
that org relationship; ensure you reuse the same checks/utility used by
canManageB2BAppPosIntegrationConfigs to avoid divergence.
apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts`:
- Around line 78-81: The hook usePosIntegrationLastTestingPosReceipt currently
returns a hardcoded stub for lastTestingPosReceipt; replace the stubbed object
(lastTestingPosReceipt: { id: 'test', condoPaymentId: '2' }) with the actual
fetched state from the query (e.g., the query response variable such as data or
lastTestingPosReceipt from your fetch), so callers receive the real receipt;
keep the existing loading and refetch values (loading: areB2bAppContextsLoading
|| loading and refetch: fetchLastTestingPosReceipt) and ensure you handle
null/undefined safely (return null or undefined when the fetch hasn’t produced a
receipt).
- Around line 31-34: The destructuring of the query result in
usePosIntegrationLastTestingPosReceipt assumes data is always defined; change
the destructure for useGetB2BAppContextWithPosIntegrationConfigQuery so it
safely handles undefined data (e.g., default data = {} or use optional chaining)
before accessing data.contexts, ensuring areB2bAppContextsLoading and
b2bAppContexts are assigned without throwing when the hook returns undefined
initially.
In
`@apps/condo/domains/acquiring/queries/B2BAppContextWithPosIntegrationConfig.graphql`:
- Around line 6-7: The query currently applies manual deletedAt filters (e.g.,
the nested filter "app: { posIntegrationConfig_is_null: false, deletedAt: null
}" and top-level "deletedAt: null"); remove these explicit deletedAt predicates
so the softDeleted plugin can handle soft-deletion automatically, leaving only
the necessary conditions like "posIntegrationConfig_is_null: false" on the app
field and deleting the two deletedAt checks in
B2BAppContextWithPosIntegrationConfig.graphql.
♻️ Duplicate comments (2)
apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts (2)
35-37: Avoid dereferencing organization before it exists.Line 36 can throw when
organizationis undefined. Use optional chaining (or delay variables) even ifskipis set. This repeats a prior review point.🛠️ Proposed fix
- variables: { organizationId: organization.id }, + variables: { organizationId: organization?.id },
56-66: Replace console.error with structured logger.Use
getLogger()and structured fields instead ofconsole.errorfor both the non-OK response and the catch block. This repeats a prior review point. As per coding guidelines, use structured logging.🛠️ Proposed fix
+import { getLogger } from '@open-condo/keystone/logging' + +const logger = getLogger('usePosIntegrationLastTestingPosReceipt') + if (!response.ok) { - console.error('Failed to fetch last receipt:', response.statusText) + logger.error({ msg: 'Failed to fetch last receipt', data: { status: response.status, statusText: response.statusText } }) setLastTestingPosReceipt(null) return } @@ } catch (error) { - console.error('Error fetching last receipt:', error) + logger.error({ msg: 'Error fetching last receipt', err: error }) setLastTestingPosReceipt(null) } finally {
apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts
Show resolved
Hide resolved
apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts
Show resolved
Hide resolved
| app: { posIntegrationConfig_is_null: false, deletedAt: null }, | ||
| deletedAt: null, |
There was a problem hiding this comment.
Remove manual deletedAt filters to preserve softDeleted behavior.
Manually filtering deletedAt disables the softDeleted plugin’s automatic handling and can unintentionally filter out nested entities. Please rely on the plugin instead. Based on learnings, do not add deletedAt filters in GraphQL queries.
🧹 Suggested fix
- app: { posIntegrationConfig_is_null: false, deletedAt: null },
- deletedAt: null,
+ app: { posIntegrationConfig_is_null: false },📝 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.
| app: { posIntegrationConfig_is_null: false, deletedAt: null }, | |
| deletedAt: null, | |
| app: { posIntegrationConfig_is_null: false }, |
🤖 Prompt for AI Agents
In
`@apps/condo/domains/acquiring/queries/B2BAppContextWithPosIntegrationConfig.graphql`
around lines 6 - 7, The query currently applies manual deletedAt filters (e.g.,
the nested filter "app: { posIntegrationConfig_is_null: false, deletedAt: null
}" and top-level "deletedAt: null"); remove these explicit deletedAt predicates
so the softDeleted plugin can handle soft-deletion automatically, leaving only
the necessary conditions like "posIntegrationConfig_is_null: false" on the app
field and deleting the two deletedAt checks in
B2BAppContextWithPosIntegrationConfig.graphql.
…ma for POS integration Added new B2BAppPosIntegrationConfig model to store POS integration configuration for B2BApps. The config includes paymentsAlertPageUrl field for displaying integration status alerts on billing and marketplace pages. Added relationship to B2BApp model, access rules, CRUD tests, and GQL queries. Only admins and support users can create/update configs, all authenticated users can read.
…ts page Added usePosIntegrationAlert hook to display POS integration status alerts on payments page. The hook fetches B2BAppContext with posIntegrationConfig and renders an IFrame with paymentsAlertPageUrl if available. Added loading state handling and onLoad callback support to IFrame component. Updated PaymentsTable to show alert above table content with loading spinner. Changed lodash import to use specific function import.
…OnOff icon Renamed variables in usePosIntegrationAlert hook for clarity: isPosIntegrationAppContextsLoading to areB2bAppContextsLoading, posIntegrationAppContexts to b2bAppContexts. Removed isIFrameHidden from loading state calculation. Fixed OnOff icon background path fill from white to none.
…pacing Updated usePosIntegrationAlert hook to include isIFrameHidden in loading state calculation. Changed PaymentsTable Space component size to be 0 during loading to prevent extra spacing when spinner is shown.
…pPosIntegrationConfig and highlight payments with receipts Added fetchLastPosReceiptUrl field to B2BAppPosIntegrationConfig schema for fetching last POS receipt data. Created usePosIntegrationLastPosReceipt hook to fetch receipt information from miniapp. Updated PaymentsTable to highlight rows with orange background when payment has associated receipt. Regenerated migration to include new field.
…hen no alert URL exists
…grationConfig test fixtures Added fetchLastPosReceiptUrl field to create and update test payloads in B2BAppPosIntegrationConfig.test.js. Updated createTestB2BAppPosIntegrationConfig helper to include fetchLastPosReceiptUrl with faker-generated URL.
…d yarn.lock Add the @open-keystone/server-side-graphql-client package to dependencies
nosemgrep added for one line
…ctual version of next)
…phQL query for POS integration config Created dedicated GraphQL query getB2BAppContextWithPosIntegrationConfig to fetch B2BAppContext with POS integration configuration. Replaced B2BAppContext.useObjects calls with useGetB2BAppContextWithPosIntegrationConfigQuery hook in usePosIntegrationAlert and usePosIntegrationLastTestingPosReceipt.
AleX83Xpert
force-pushed
the
feat/pos-integration/doma-12621/inject-pos-integration-to-condo
branch
from
4d20ace to
62e186a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/condo/domains/acquiring/hooks/usePosIntegrationAlert.tsx`:
- Around line 18-26: The hook call to
useGetB2BAppContextWithPosIntegrationConfigQuery unsafe-destructures data and
references organization.id too early; change the call so variables are only
supplied when organization?.id exists (e.g., compute variables =
organization?.id ? { organizationId: organization.id } : undefined and pass
skip: !organization?.id), and stop destructuring directly from data — read
data?.contexts or provide a default like const b2bAppContexts = data?.contexts
?? []; update references to use b2bAppContexts and ensure organization?.id is
used where checked by skip.
♻️ Duplicate comments (9)
apps/condo/domains/miniapp/access/B2BAppPosIntegrationConfig.js (1)
10-15: Read access is overly permissive compared to manage access.Returning
{}grants any authenticated user global read access to all POS integration configs, while the manage function properly restricts access. This inconsistency could leak integration URLs across organizations.Consider aligning read access with manage access by checking for support/admin or delegating to
canDirectlyManageSchemaObjects.apps/condo/lang/es/es.json (1)
2427-2430: Align tone with the rest of the Spanish UI (tú vs. usted).
These strings use formal address; most UI uses informal “tú”. Suggest switching to informal for consistency.✏️ Proposed text update
-"pages.condo.payments.posReceiptVerifyTitle": "Verifique la exactitud del recibo", -"pages.condo.payments.posReceiptVerifyDescription": "Si todos los datos son correctos, puede activar la fiscalización. Si hay errores, contacte al soporte técnico.", +"pages.condo.payments.posReceiptVerifyTitle": "Verifica la exactitud del recibo", +"pages.condo.payments.posReceiptVerifyDescription": "Si todos los datos son correctos, puedes activar la fiscalización. Si hay errores, contacta al soporte técnico.",apps/condo/domains/acquiring/hooks/usePosIntegrationLastTestingPosReceipt.ts (4)
31-38: Guard organization access before building variables.Line 36 dereferences
organization.ideven whenorganizationmay be undefined, which can throw beforeskiptakes effect. Use optional chaining for the variables payload.🛠️ Proposed fix
- variables: { organizationId: organization.id }, + variables: { organizationId: organization?.id },
31-34: Avoid deep destructuring of query data without a default.Line 33 assumes
datais always defined. Apollo can returndata: undefinedon initial render, which will throw when destructuring.🛠️ Proposed fix
- data: { contexts: b2bAppContexts }, + data: { contexts: b2bAppContexts } = {},
56-66: Use structured logger instead of console.error.Replace
console.errorwith the repo logger and structured fields.🧭 Suggested fix
import { useCallback, useEffect, useState } from 'react' +import { getLogger } from '@open-condo/keystone/logging' import { useOrganization } from '@open-condo/next/organization' import { B2BAppContext } from '@condo/domains/miniapp/utils/clientSchema' +const logger = getLogger('usePosIntegrationLastTestingPosReceipt') + @@ - if (!response.ok) { - console.error('Failed to fetch last receipt:', response.statusText) + if (!response.ok) { + logger.error({ msg: 'Failed to fetch last receipt', data: { status: response.status, statusText: response.statusText } }) setLastTestingPosReceipt(null) return } @@ - } catch (error) { - console.error('Error fetching last receipt:', error) + } catch (error) { + logger.error({ msg: 'Error fetching last receipt', err: error }) setLastTestingPosReceipt(null) } finally {As per coding guidelines, use getLogger with structured fields.
78-81: Return the fetched receipt instead of a stub.Line 79 hardcodes a test object, so callers never see the real receipt state.
🛠️ Proposed fix
return { - lastTestingPosReceipt: { id: 'test', condoPaymentId: '2' }, + lastTestingPosReceipt, loading: areB2bAppContextsLoading || loading, refetch: fetchLastTestingPosReceipt, }apps/condo/domains/acquiring/queries/B2BAppContextWithPosIntegrationConfig.graphql (1)
6-7: Remove manualdeletedAtfilters to preserve softDeleted behavior.The manual
deletedAt: nullfilters on the nestedappfilter and at the context level disable the softDeleted plugin's automatic handling. Based on learnings, rely on the plugin instead and remove these explicit filters.🧹 Suggested fix
- app: { posIntegrationConfig_is_null: false, deletedAt: null }, - deletedAt: null, + app: { posIntegrationConfig_is_null: false },apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx (1)
129-133: Redundant refetch on mount.
PaymentsTableContentonly mounts onceareAlertLoadingis false, so the hook already fetches on mount. This effect triggers a second, immediate refetch which is unnecessary.✅ Proposed fix: remove the effect
- useEffect(() => { - if (!areAlertLoading) { - refetchLastTestingPosReceipt() - } - }, [areAlertLoading, refetchLastTestingPosReceipt])apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx (1)
153-157: Remove the redundant refetch effect.Since
MarketplacePaymentsTableContentonly mounts afterareAlertLoadingbecomesfalse, theusePosIntegrationLastTestingPosReceipthook already fetches on mount. This effect immediately triggers a duplicate refetch.✅ Proposed fix
- useEffect(() => { - if (!areAlertLoading) { - refetchLastTestingPosReceipt() - } - }, [areAlertLoading, refetchLastTestingPosReceipt])
🧹 Nitpick comments (2)
apps/condo/domains/acquiring/components/payments/PaymentsTable.tsx (1)
297-301:onRowcallback should return a value for all paths.When the condition is false, the function implicitly returns
undefined. While this works, it's cleaner to explicitly return an empty object for consistency.♻️ Suggested improvement
onRow={(record) => { if (lastTestingPosReceipt && lastTestingPosReceipt?.condoPaymentId === record.id) { return { style: { backgroundColor: colors.orange[1] } } } + return {} }}apps/condo/domains/marketplace/components/MarketplacePaymentsContent.tsx (1)
361-365: Remove redundant optional chaining.After the truthy check
lastTestingPosReceipt &&, the optional chaining?.onlastTestingPosReceipt?.condoPaymentIdis unnecessary.✅ Proposed fix
onRow={(record) => { - if (lastTestingPosReceipt && lastTestingPosReceipt?.condoPaymentId === record.id) { + if (lastTestingPosReceipt && lastTestingPosReceipt.condoPaymentId === record.id) { return { style: { backgroundColor: colors.orange[1] } } } }}
Icon imported from Figma by command: ```sh yarn workspace @open-condo/icons import:figma ```
…ub in POS integration hooks Added default empty object destructuring for data in useGetB2BAppContextWithPosIntegrationConfigQuery to prevent errors when data is undefined. Removed hardcoded test stub from usePosIntegrationLastTestingPosReceipt return value.
…integration hooks
Changed destructured `data: { contexts: b2bAppContexts }` to `data: b2bAppContextsResult` and updated property access to `b2bAppContextsResult?.contexts` in usePosIntegrationAlert and usePosIntegrationLastTestingPosReceipt hooks for better code clarity.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…ranch was rebased)
|
AleX83Xpert
deleted the
feat/pos-integration/doma-12621/inject-pos-integration-to-condo
branch
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.