-
Notifications
You must be signed in to change notification settings - Fork 3
new graphql mutation and query for upgrading to pro/merchant account #224
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
base: main
Are you sure you want to change the base?
Conversation
|
@brh28 this still needs to be wired up to the ErpNext support issue structure (WIP) |
0f9f6cb to
f94aa26
Compare
brh28
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.
Have you tested this code against ErpNext? I'm wondering if the read/writes using the subject field is working correctly
| if (account instanceof Error) return account | ||
|
|
||
| // Validate requested level | ||
| const checkedLevel = checkedToAccountLevel(level) |
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.
lines 43-54 could be combined into a single function. something like:
const checkedLevel = validate(level, [ isValid, isGreaterThan(account.level)] )
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.
updated
|
|
||
| if (issueResult instanceof Error) return issueResult | ||
|
|
||
| // If level is 2 (Pro), auto-upgrade immediately |
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.
For level 2, why create an issue in ErpNext if we're already upgrading them?
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.
so we can have the update records in ErpNext, with timestamps, and saving their bank account information if they enter it, since it is optional for level 2.
src/services/frappe/ErpNext.ts
Outdated
| /** | ||
| * Create a support Issue in ERPNext for account upgrade requests | ||
| */ | ||
| async createSupportIssue(data: { |
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 recommend changing the interface so that the caller doesn't need to know the implementation details. For example:
createAccountUpgradeRequest(username, details)
getAccountUpgradeRequest(username)
This way, all the logic for handling upgrade requests is contained in this file, rather than spread through the code.
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.
@Nodirbek75 this is where the account upgrade request is made
|
doctype: Account Upgrade Request |
dev/config/base-config.yaml
Outdated
| # get from ErpNext user -> settings -> API Access | ||
| apiKey: "" | ||
| apiSecret: "" | ||
| apiKey: "293bac5aa957da3" |
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.
remove
| helmRevision: Int | ||
| } | ||
|
|
||
| input BusinessAccountUpgradeRequestInput { |
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 be updated to include all required fields
| if (requestResult instanceof Error) return requestResult | ||
|
|
||
| // Level 2 (Pro) auto-upgrades immediately | ||
| if (checkedLevel === 2) { |
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.
perhaps use AccountLevel enum: e.g AccountLevel.Pro
| const erpUsd = (usd: USDAmount): number => Number(usd.asCents(2)) // Number(usd.asDollars(2)) | ||
| const erpUsd = (usd: USDAmount): number => Number(usd.asCents(2)) | ||
|
|
||
| type ErpLevelString = "ZERO" | "ONE" | "TWO" | "THREE" |
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 be AccountLevels?
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.
updated. I had to map the numbers to Erp strings
|
works™ |
2e2420c to
96ee680
Compare
Mutation:
Query: