New: handle oneZero scrape#628
New: handle oneZero scrape#628shaiu wants to merge 2 commits intobrafdlog:feature/one-zerop-bankfrom
Conversation
WalkthroughThe pull request introduces enhancements to the configuration management and transaction import processes, with a focus on two-factor authentication handling. A new function Changes
Sequence DiagramsequenceDiagram
participant Scraper
participant EventPublisher
participant IPC
participant ConfigManager
Scraper->>EventPublisher: Trigger Two-Factor Auth
EventPublisher->>IPC: Request OTP
IPC-->>EventPublisher: Receive OTP
Scraper->>Scraper: Get Long-Term Token
Scraper->>ConfigManager: Update OTP Long-Term Token
ConfigManager-->>Scraper: Token Updated
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull request has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/main/src/backend/import/bankScraper.ts (2)
9-9: Type safety in.map(...)
Castingkey as CompanyTypesis reasonable. If the codebase expands or new keys are introduced, consider adding runtime checks to avoid potential type mismatches.
38-38: Progress event callback
Emitting concise progress messages keeps the UI responsive. Consider logging or storing payload details if deeper analysis is needed later.packages/renderer/src/components/App.tsx (1)
13-13: RenderGetOtp
Placing<GetOtp />within the main app ensures the OTP modal is always available. Ensure that the modal’s visibility logic remains controlled via the store to avoid unexpected pop-ups.packages/main/src/backend/configManager/configManager.ts (1)
39-54: Consider enhancing type safety and validation.While the implementation is generally good, there are a few improvements to consider:
- The type assertion on line 45-47 could be replaced with a more precise type definition
- Consider validating the
otpLongTermTokenformat/structure before updatingHere's a suggested improvement:
+interface AccountWithLoginFields { + key: CompanyTypes; + loginFields: { + otpLongTermToken: string; + }; +} export async function updateOtpLongTermToken( key: CompanyTypes, otpLongTermToken: string, configPath: string = configFilePath, ): Promise<void> { const config = await getConfig(configPath); - const account = config.scraping.accountsToScrape.find((acc) => acc.key === key) as { - loginFields: { otpLongTermToken: string }; - }; + const account = config.scraping.accountsToScrape.find((acc): acc is AccountWithLoginFields => + acc.key === key && 'loginFields' in acc && 'otpLongTermToken' in acc.loginFields + ); if (account) { + if (!otpLongTermToken || typeof otpLongTermToken !== 'string') { + throw new Error('Invalid OTP long-term token format'); + } account.loginFields.otpLongTermToken = otpLongTermToken; await updateConfig(configPath, config); } else { throw new Error(`Account with key ${key} not found`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/main/src/backend/configManager/configManager.ts(2 hunks)packages/main/src/backend/import/bankScraper.ts(2 hunks)packages/main/src/backend/import/importTransactions.ts(3 hunks)packages/main/src/manual/setupHelpers.ts(0 hunks)packages/renderer/src/components/App.tsx(2 hunks)packages/renderer/src/components/GetOtp.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/main/src/manual/setupHelpers.ts
🔇 Additional comments (16)
packages/main/src/backend/import/bankScraper.ts (10)
2-2: Add clarity on usage ofSCRAPERSimports
No functional issues identified. If future expansions require customizing scrapers, keep the import path consistent and well-documented.
3-3: Validate electron IPC usage
ImportingipcMainis appropriate here, but ensure usage remains restricted to the main process context. Consider future modularization if more IPC functionalities appear.
4-4: Confirm event-based architecture
IntroducingEventPublisherfromEventEmitteris a good approach for decoupling event dispatch logic from the core scraping flow. No immediate issues.
5-5: Centralize OTP token updates
UsingupdateOtpLongTermTokenfrom the config manager is clean and maintainable. This ensures OTP management is centralized.
25-25: NeweventPublisherparameter
AddingeventPublisheris a neat way to handle external event emission. Ensure every call site passes a validEventPublisherinstance.
28-32: Scraperoptionsstructure
Good to see the relevant fields (companyId, startDate, etc.) consolidated here. Settingverbose: falseby default may hamper debugging; ensure it meets project requirements.
36-36: Empty line
No meaningful change.
39-39: Empty line
No meaningful change.
40-56: Implement 2FA flow for oneZero
This block introduces a well-structured approach for OTP retrieval and long-term token generation. Pointers:
- Consider encrypting the
otpLongTermTokenif stored on disk.- Handle user inactivity or canceled OTP entry gracefully.
Overall, logic is sound and extends the scraper flow robustly.
58-58: Return scraper results
Returningawait scraper.scrape(credentials)is appropriate for an async function. Ensure error handling from the scraper is surfaced for debugging.packages/renderer/src/components/App.tsx (1)
4-4: ImportGetOtp
Neatly imported from./GetOtp. This separation of OTP logic into its own component helps keepAppcleaner.packages/renderer/src/components/GetOtp.tsx (1)
4-4: Use of relative import
Switching to a relative path foruseConfigStoreconfirms the code structure. Verify consistency across the codebase for store imports.packages/main/src/backend/configManager/configManager.ts (1)
7-7: LGTM!The import of
CompanyTypesfrom 'israeli-bank-scrapers-core' is appropriate for type safety.packages/main/src/backend/import/importTransactions.ts (3)
17-17: LGTM!The addition of
ImporterEventto the imports is appropriate for handling OTP-related events.
120-120: LGTM!Adding the
eventPublisherparameter tobankScraper.scrapeenables proper event handling for OTP flows.
172-177: LGTM!The simplified return statement improves code readability without changing functionality.
baruchiro
left a comment
baruchiro
left a comment
There was a problem hiding this comment.
Didn't finished the review, הילדה שלי התעוררה 🤷♂️
BTW, I don't remember, but are you using the OTP support introduced on israeli-bank-scrapers here eshaham/israeli-bank-scrapers#760?
| export async function updateOtpLongTermToken( | ||
| key: CompanyTypes, | ||
| otpLongTermToken: string, | ||
| configPath: string = configFilePath, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
rename to companyId
| export async function updateOtpLongTermToken( | |
| key: CompanyTypes, | |
| otpLongTermToken: string, | |
| configPath: string = configFilePath, | |
| ): Promise<void> { | |
| export async function updateOtpLongTermToken( | |
| companyId: CompanyTypes, | |
| otpLongTermToken: string, | |
| configPath: string = configFilePath, | |
| ): Promise<void> { |
| if (account) { | ||
| account.loginFields.otpLongTermToken = otpLongTermToken; | ||
| await updateConfig(configPath, config); | ||
| } else { | ||
| throw new Error(`Account with key ${key} not found`); | ||
| } |
There was a problem hiding this comment.
return early: if not account, throw. Otherwise, continue.
| if (account) { | |
| account.loginFields.otpLongTermToken = otpLongTermToken; | |
| await updateConfig(configPath, config); | |
| } else { | |
| throw new Error(`Account with key ${key} not found`); | |
| } | |
| if (!account) { | |
| throw new Error(`Account with key ${key} not found`); | |
| } | |
| account.loginFields.otpLongTermToken = otpLongTermToken; | |
| await updateConfig(configPath, config); |
| export const inputVendors = Object.keys(SCRAPERS) | ||
| // Deprecated. see https://github.com/eshaham/israeli-bank-scrapers/blob/07ecd3de0c4aa051f119aa943493f0cda943158c/src/definitions.ts#L26-L29 | ||
| .filter((key) => key !== CompanyTypes.hapoalimBeOnline) | ||
| .map((key) => ({ | ||
| key, | ||
| ...SCRAPERS[key as CompanyTypes], | ||
| })); | ||
| .map((key) => ({ key, ...SCRAPERS[key as CompanyTypes] })); | ||
|
|
There was a problem hiding this comment.
don't remove this comment
| if (companyId === CompanyTypes.oneZero) { | ||
| const creds = credentials as typeof credentials & { otpLongTermToken: string; phoneNumber: string }; | ||
| if (!creds.otpLongTermToken) { | ||
| await scraper.triggerTwoFactorAuth(creds.phoneNumber); | ||
| await eventPublisher.emit(EventNames.GET_OTP); | ||
| const otpCode = await new Promise<string>((resolve) => { | ||
| ipcMain.once('get-otp-response', (event, input) => resolve(input)); | ||
| }); | ||
| const result = await scraper.getLongTermTwoFactorToken(otpCode); | ||
| if ('longTermTwoFactorAuthToken' in result) { | ||
| await updateOtpLongTermToken(companyId, result.longTermTwoFactorAuthToken); | ||
| creds.otpLongTermToken = result.longTermTwoFactorAuthToken; | ||
| } else { | ||
| throw new Error('Failed to get long-term two-factor auth token'); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function must be extracted into a function. More than that, think when more banks will require OTP, I expect the "if" to be extracted into a function to.
I hope you understand what I mean. Here should be only handleOtp function, and inside it there will be if-else/switch-case that will direct into bank-specific function.
| await scraper.triggerTwoFactorAuth(creds.phoneNumber); | ||
| await eventPublisher.emit(EventNames.GET_OTP); |
There was a problem hiding this comment.
- Use
emitProgressEventinstead ofeventPublisher.emit. - Is the
scrapernot emitting events of OTP itself?
| const otpCode = await new Promise<string>((resolve) => { | ||
| ipcMain.once('get-otp-response', (event, input) => resolve(input)); | ||
| }); |
There was a problem hiding this comment.
All IPC calls are declared here: packages/main/src/handlers/index.ts
I expect this call to follow that pattern.
| if (companyId === CompanyTypes.oneZero) { | ||
| const creds = credentials as typeof credentials & { otpLongTermToken: string; phoneNumber: string }; | ||
| if (!creds.otpLongTermToken) { | ||
| await scraper.triggerTwoFactorAuth(creds.phoneNumber); |
There was a problem hiding this comment.
I'm not sure how the process is implemented, but I see the triggerTwoFactorAuth is called during the scraping on israeli-bank-scrapers, see triggerTwoFactorAuth, so why running it in our code too?
| const result = await scraper.getLongTermTwoFactorToken(otpCode); | ||
| if ('longTermTwoFactorAuthToken' in result) { | ||
| await updateOtpLongTermToken(companyId, result.longTermTwoFactorAuthToken); | ||
| creds.otpLongTermToken = result.longTermTwoFactorAuthToken; | ||
| } else { | ||
| throw new Error('Failed to get long-term two-factor auth token'); | ||
| } |
There was a problem hiding this comment.
use result.success
| const result = await scraper.getLongTermTwoFactorToken(otpCode); | |
| if ('longTermTwoFactorAuthToken' in result) { | |
| await updateOtpLongTermToken(companyId, result.longTermTwoFactorAuthToken); | |
| creds.otpLongTermToken = result.longTermTwoFactorAuthToken; | |
| } else { | |
| throw new Error('Failed to get long-term two-factor auth token'); | |
| } | |
| const result = await scraper.getLongTermTwoFactorToken(otpCode); | |
| if (result.success) { | |
| await updateOtpLongTermToken(companyId, result.longTermTwoFactorAuthToken); | |
| creds.otpLongTermToken = result.longTermTwoFactorAuthToken; | |
| } else { | |
| throw new Error('Failed to get long-term two-factor auth token'); | |
| } |
| } | ||
|
|
There was a problem hiding this comment.
What is it? I see it is unused..?
If it is not related to your work, I prefer you to open a new PR for this, so if there will be a problem with this remove, it will be easy to revert it.
| const hash = calculateTransactionHash(transaction, companyId, accountNumber); | ||
| // const category = categoryCalculation.getCategoryNameByTransactionDescription(transaction.description); | ||
| const enrichedTransaction: EnrichedTransaction = { | ||
| return { | ||
| ...transaction, | ||
| accountNumber, | ||
| // category, |
There was a problem hiding this comment.
Oh I see we don't have a category calculation anymore.
Anyway, please return the comment back.
Yes, I agree that developer should avoid code comments, but this project is in low maintainance and I prefer to keep such comments for the next time I will search something.
|
Please pull the updates from the feature branch |
|
Pull request has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
2 participants
Summary by CodeRabbit
Release Notes
New Features
GetOtpcomponent for handling OTP inputImprovements
Removed
The updates focus on improving authentication and scraping processes for bank transactions.