Skip to content

Comments

Feat/update ppiurl bankaccountverification#41

Open
nitika1995-dev wants to merge 4 commits intoekoindia:masterfrom
nitika1995-dev:feat/updatePPIURL-bankaccountverification
Open

Feat/update ppiurl bankaccountverification#41
nitika1995-dev wants to merge 4 commits intoekoindia:masterfrom
nitika1995-dev:feat/updatePPIURL-bankaccountverification

Conversation

@nitika1995-dev
Copy link
Collaborator

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation Update (if none of the other choices apply)

🚨 Checklist:

  • Make sure you are requesting to pull your topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against our dev branch (left side). Also, you should start your branch off our dev branch.
  • Your code follows the code style of this project.
  • Your change requires a change to the documentation.
  • You have updated the documentation accordingly.
  • You have read the Contributing Guide.
  • You have self-reviewed your code.
  • You have added unit-tests that prove my fix is effective or that my feature works.
  • Lint and unit-tests pass locally with your changes.
  • You have added necessary comments for complex logic.
  • Any dependent changes have been merged and published in downstream modules.

Further comments

🙏 Thank you!

Thank you for contributing to this project. We appreciate your time and effort. 🎉

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @nitika1995-dev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team, gemini-code-assist here to provide a summary of this pull request. This PR primarily focuses on updating the API documentation in the README.md file. The changes involve renaming several API endpoints related to PPI (Prepaid Instrument) features, adding documentation for a new Bank Account Verification API, and making minor adjustments to existing API descriptions.

Highlights

  • API Endpoint Renaming: Several API endpoints documented in the README have been updated, changing the path segment /ppi/ to /ppi-digikhata/. This affects endpoints for OTP verification, Aadhaar verification, PAN verification, listing recipients, adding recipients, adding recipient banks, sending transaction OTPs, and initiating PPI transactions.
  • New Bank Account Verification API: Documentation for a new synchronous Bank Account Verification API (/tools/kyc/bank-account/sync) has been added. This API allows verifying a bank account by transferring ₹1 and retrieving the account holder's name, along with sample success and failure responses.
  • Add Recipient API Updates: The documentation for the 'Add Recipient' API has been updated to include two new required body parameters: account_type and type.
  • Documentation Structure Changes: Due to the addition of the new Bank Account Verification API, the subsequent sections in the README related to bulk verification and IFSC verification have been renumbered.
  • Potential Merge Conflict in Recipient List API Method: Note that there appears to be a merge conflict marker (<<<<<<<, =======, >>>>>>>) in the documentation for the 'List Recipients' API, indicating a change in the documented method from GET to POST. This should be resolved.

Changelog

Click here to see the changelog
  • markdown/README.md
    • Updated URL endpoint for OTP verification from /ppi/otp/verify to /ppi-digikhata/otp/verify (Line 701).
    • Updated URL endpoint for Aadhaar verification from /ppi/aadhaar to /ppi-digikhata/aadhaar (Line 793).
    • Updated URL endpoint for PAN verification from /ppi/pan to /ppi-digikhata/pan (Line 824).
    • Updated URL endpoint for listing recipients from /customer/payment/ppi/sender/{customer_id}/recipients to /customer/payment/ppi-digikhata/sender/{customer_id}/recipients (Line 897).
    • Introduced merge conflict markers around the HTTP method for the recipient list API, changing from GET to POST (Lines 892-896).
    • Updated URL endpoint for adding recipients from /customer/payment/ppi/sender/{customer_id}/recipient to /customer/payment/ppi-digikhata/sender/{customer_id}/recipient (Line 988).
    • Added account_type and type as required parameters for the Add Recipient API (Lines 1002-1003).
    • Updated description for 'Add Recipient Bank API' and changed its URL endpoint from /customer/payment/ppi/sender/{customer_id}/bank/recipient to /customer/payment/ppi-digikhata/sender/{customer_id}/recipient/bank (Lines 1027, 1032).
    • Updated URL endpoint for sending transaction OTP from /customer/payment/ppi/otp to /customer/payment/ppi-digikhata/otp (Line 1067).
    • Updated URL endpoint for initiating PPI transaction from /customer/payment/ppi to /customer/payment/ppi-digikhata (Line 1098).
    • Added documentation for a new 'Bank Account Verification API' (/tools/kyc/bank-account/sync) including details and sample responses (Lines 3799-3862).
    • Renumbered 'Bulk Bank Account Verification (Async) API' from 1.3 to 1.4 (Line 3864).
    • Renumbered 'Bulk Bank Account Verification Status API' from 1.4 to 1.5 (Line 3889).
    • Renumbered 'IFSC Verification API' from 1.5 to 1.6 (Line 3949).
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Docs updated, paths now clear,
Digikhata, have no fear.
New API, sync and fast,
Bank accounts verified at last.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates several PPI URL endpoints in the README.md and introduces a new 'Bank Account Verification API' section. The changes aim to keep the API documentation current.

However, the pull request description is currently a template and hasn't been filled out. Could you please provide details on the changes, their motivation and context, and how they've been tested? This information is crucial for a comprehensive review and for future reference.

Summary of Findings

  • PR Description Missing: The pull request description is currently empty. Filling it out with details about the changes, motivation, and testing performed would greatly help reviewers understand the context and impact of this PR.
  • Critical Merge Conflict: A critical merge conflict exists in markdown/README.md (around lines 892-896 on the right side of the diff) that needs to be resolved.
  • Parameter Clarity (type): The type parameter in the 'Add/Update Recipient API' (line 1003) has a confusing description (Send a fixed value of acc_ifsc) for an int type, potentially leading to misinterpretation. This needs clarification on whether it should be a string or if the integer value and its meaning should be explicitly stated.
  • Parameter Clarity (source): The source parameter description in the new 'Bank Account Verification API' (line 3811) could be more precise. If 'API' is a fixed value, it should be stated clearly.
  • Parameter Definition (bank_account): The bank_account parameter in the new 'Bank Account Verification API' (line 3814) needs clarification on whether it's required or optional. Its description could also be more formal.
  • Minor Documentation Polish (Not Commented In-line): There are minor typos (e.g., 'registered' instead of 'register' on line 1027) and informal phrasing (e.g., 'need to pass the complete value' on line 3813) in the documentation. These were not commented on directly due to review settings but are worth correcting for overall quality and professionalism of the documentation.

Merge Readiness

This pull request aims to update PPI URLs and introduce a new Bank Account Verification API in the documentation. While these are valuable updates, there are several issues that need to be addressed before this PR is ready for merging:

  1. Critical Merge Conflict: There's an unresolved merge conflict in markdown/README.md that must be fixed.
  2. Missing PR Description: The pull request lacks a description, which is essential for understanding the context and purpose of the changes.
  3. API Parameter Clarity: Several API parameter descriptions (for type, source, and bank_account) require clarification to ensure accuracy and prevent misinterpretation by users of the API documentation.

Due to these issues, particularly the critical merge conflict, I recommend that these changes be made before the PR is considered for merging. I am not authorized to approve pull requests, but addressing these points will significantly improve the quality and readiness of this documentation update. It should then be re-reviewed and approved by authorized team members.

Comment on lines +892 to +896
<<<<<<< HEAD
- **Method:** GET
- **URL Endpoint:** /customer/payment/ppi/sender/{customer_id}/recipients
=======
- **Method:** POST
>>>>>>> b7f871a3616ae18674dfa49a9ab3ed7dfc08e656

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

It appears there are unresolved merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>) in this section. These need to be resolved before the PR can be merged.

Additionally, these markers seem to indicate a potential change in the HTTP method for the /customer/payment/ppi-digikhata/sender/{customer_id}/recipients endpoint (from GET to POST). Could you confirm if this method change is intentional and ensure the final version accurately reflects the correct HTTP method after resolving the conflict?

- **bank_code** (string / required) - The IFSC code of the recipient's bank branch.
- **service_code** (int / required) - For PayPoint,send a fixed value of 80.
- **account_type** (int / required) - For PayPoint,send a fixed value of 1.
- **type** (int / required) - Send a fixed value of acc_ifsc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The description for the type parameter is Send a fixed value of acc_ifsc, but its data type is specified as int. This is potentially confusing and could lead to incorrect API usage by consumers of this documentation.

  • If acc_ifsc is a literal string value that should be sent, should the type be (string / required) and the description be something like Fixed value: "acc_ifsc"?
  • Or, if type is indeed an integer that represents the concept of 'acc_ifsc' (e.g., an enum value mapped to an integer), could the description be clarified to specify the exact integer value and what 'acc_ifsc' signifies in this context? For example: (int / required) - Specifies the type of operation. Send the integer value X, which represents 'acc_ifsc'.

Please clarify the intended data type, value, and meaning for this parameter.

- **Body Parameters:**
- initiator_id (string / required) - Your registered mobile number (See Platform Credentials for UAT)
- user_code (string / required) - User code value of the retailer from whom the request is coming
- source (string / required) - Enter the value of source=API

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description for the source parameter is Enter the value of source=API. If 'API' is the only expected fixed value for this parameter, could this be made more explicit for clarity? For instance, by stating Fixed value: "API".

If other values are possible, perhaps a more general description like Source of the request (e.g., "API", "WEB"). would be more appropriate. Could you clarify the expected value(s)?

Suggested change
- source (string / required) - Enter the value of source=API
- source (string / required) - Source of the request. For this API, send the fixed value "API".

- source (string / required) - Enter the value of source=API
- client_ref_id (string / required) - A unique ID for every API call generated at your end
- ifsc (string / required) - need to pass the complete value of IFSC code
- bank_account (string) - pass complete account number which needs to be verified

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The bank_account parameter is listed as (string) without specifying if it's required or optional (e.g., string / required or string / optional). Given its role in bank account verification, it seems likely to be a required field. Could you please clarify this and update the documentation accordingly?

Additionally, the description pass complete account number which needs to be verified is a bit informal. Would The complete bank account number to be verified. be a clearer and more formal alternative?

Suggested change
- bank_account (string) - pass complete account number which needs to be verified
- bank_account (string / required) - The complete bank account number which needs to be verified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant