started adding transmittable for the VLP REST XML#176
started adding transmittable for the VLP REST XML#176kristinmerbach wants to merge 6 commits intotrunkfrom
Conversation
|
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe updates involve the introduction of a new Changes
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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (8)
- app/models/transmittable/person.rb (1 hunks)
- app/models/transmittable/transaction.rb (1 hunks)
- app/operations/fdsh/jobs/generate_transmittable_vlp_payload.rb (1 hunks)
- app/operations/fdsh/vlp/rx92/handle_initial_verification_request.rb (1 hunks)
- app/operations/fdsh/vlp/rx92/process_initial_verification_response.rb (1 hunks)
- app/operations/fdsh/vlp/rx92/request_vlp_verification.rb (1 hunks)
- app/operations/fdsh/vlp/rx92/transform_person_to_initial_request.rb (1 hunks)
- app/operations/fdsh/vlp/rx92/transform_person_to_xml_request.rb (1 hunks)
Files skipped from review due to trivial changes (1)
- app/operations/fdsh/vlp/rx92/request_vlp_verification.rb
Additional comments: 28
app/models/transmittable/transaction.rb (5)
37-37: The addition of the
xml_payloadfield to theTransactionclass aligns with the PR objectives to enhance VLP REST XML functionality.37-37: Verify that
::Transmittable::Transmission.define_transmission_constantsis called before thetransmit_action=andstatus=methods to ensure the constants are defined.37-37: Ensure that the
transactions_transmissionsassociation and thetransmissionmethod inTransactionsTransmissionsare correctly implemented to support the logic in thetransmissionmethod.37-37: Confirm that all possible
transactableobjects have either anhbx_idor anhbx_assigned_idattribute to prevent potential NoMethodError in thesubject_hbx_idmethod.37-37: Verify that the
errorsmethod exists and thattransmittable_errorsis an enumerable collection that responds tomapto support theerror_messagesmethod.app/operations/fdsh/jobs/generate_transmittable_vlp_payload.rb (8)
9-17: The main
callmethod orchestrates the payload generation process. It sequentially calls private methods to validate parameters, create a job, create a transmission, create a person subject, create a transaction, and generate a transmittable payload. The flow appears logical and follows the expected steps for the operation.21-26: The
validate_paramsmethod checks for the presence and type of required parameters. It's important to ensure that all callers of this operation are updated to pass the correct types and that the parameters are sanitized before being passed to this method to prevent any security issues.31-40: The
create_jobmethod delegates job creation toFdsh::Jobs::FindOrCreateJoband handles the result. It's crucial to ensure thatgenerate_message_idon line 36 is idempotent or that it's acceptable to change the message ID if the job already exists.43-50: The
create_transmissionmethod creates a transmission and handles errors by updating the status and adding errors. It's important to verify that theupdate_statusandadd_errorsmethods are correctly logging and handling errors, and that the error messages do not leak any PII.60-81: The
create_person_subjectmethod looks for an existing person bycorrelation_idand creates a new one if not found. It's important to ensure that thecorrelation_idis indexed for performance and that theencrypted_ssnis handled securely throughout the system.84-93: The
create_transactionmethod creates a transaction and handles errors similarly to thecreate_transmissionmethod. It's important to verify that the transaction creation process is atomic and that any failure in this process does not leave the system in an inconsistent state.96-118: The
generate_transmittable_payloadmethod transforms the payload to XML and updates the transaction. It's important to ensure that the transformation process is secure and that the XML payload does not contain any sensitive information unless it's properly encrypted or sanitized.121-134: The
transmittablemethod checks if the transaction has an XML payload and a message ID, then returns success or failure accordingly. It's important to ensure that the message ID generation is secure and that it cannot be exploited to infer or predict other message IDs.app/operations/fdsh/vlp/rx92/handle_initial_verification_request.rb (10)
11-26: The main
callmethod orchestrates the VLP request handling process. It appears to be well-structured, chaining operations with monadicyieldstatements for a clear flow of data and error handling.30-34: The
validate_paramsmethod checks for the presence ofcorrelation_idandpayload. It's important to ensure that all required parameters are validated here to prevent processing incomplete or invalid requests.37-46: The
transmittable_payloadmethod delegates toGenerateTransmittableVlpPayloadand handles the result. It's crucial thatGenerateTransmittableVlpPayloadis thoroughly tested to ensure it can handle all expected input cases.49-63: The
generate_jwtmethod generates a JWT and updates the status based on the result. It's important to verify that theJwt::GetJwtservice is robust and that the error handling here is comprehensive.66-82: The
publish_vlp_requestmethod sends the VLP request and updates the status accordingly. It's critical to ensure that theRequestVlpVerificationservice is reliable and that the error messages are informative for debugging purposes.85-105: The
create_response_transmissionmethod creates a transmission record for the response. It's important to ensure that theCreateTransmissionservice is well-tested and that the error handling is sufficient.108-131: The
create_response_transactionmethod creates a transaction record for the response and assigns the XML payload. It's important to verify that the XML payload is being assigned correctly and that the transaction is saved properly.155-170: The
process_responsemethod processes the VLP response. It's critical to ensure that theProcessInitialVerificationResponseservice is thoroughly tested and that the status updates and error handling are robust.174-185: The
build_eventandpublishmethods are responsible for building and publishing the event after processing the VLP response. It's important to ensure that the event payload is structured correctly and that the event is published successfully.188-193: The
add_errorsandupdate_statusmethods are utility methods for error logging and status updating. It's important to verify that these methods are called with the correct parameters and that they function as expected in all scenarios.app/operations/fdsh/vlp/rx92/transform_person_to_xml_request.rb (5)
13-19: The
callmethod orchestrates the transformation of a person's JSON payload to XML. It sequentially parses JSON, validates the person hash, builds a person object, transforms it to an initial request, and finally encodes it to XML. Ensure that each step includes adequate error handling and that the transformations align with the expected VLP XML format.33-35: The comment on line 34 suggests that the
encode_xml_and_schema_validatemethod is a placeholder and will be replaced. Ensure that the replacement is tracked and implemented before finalizing the feature to avoid deploying temporary code to production.38-46: The
encode_request_xmlmethod encodes the XML string and sets the encoding to 'UTF-8'. Ensure that the encoding specified here is consistent with the rest of the system and that the XML declaration is intentionally omitted as per the requirements of the VLP system.49-52: The
validate_person_json_hashmethod usesAcaEntities::Contracts::People::PersonContractfor validation. Ensure that the contract is up-to-date with the latest VLP requirements and that all necessary fields are validated.55-62: The
build_personmethod constructs aPersonobject from the validated hash. Ensure that thePersonentity is correctly mapped to the VLP requirements and that all necessary attributes are included.
| Failure( | ||
| "Invalid person, given value is not a ::AcaEntities::People::Person, input_value:#{person}" |
There was a problem hiding this comment.
The error message in the Failure response could be more informative by including the actual class of the input object when it's not a ::AcaEntities::People::Person.
- "Invalid person, given value is not a ::AcaEntities::People::Person, input_value:#{person}"
+ "Invalid person, given value is not a ::AcaEntities::People::Person, input_value:#{person.class}"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.
| Failure( | |
| "Invalid person, given value is not a ::AcaEntities::People::Person, input_value:#{person}" | |
| Failure( | |
| "Invalid person, given value is not a ::AcaEntities::People::Person, input_value:#{person.class}" |
| # This class takes a json representing a person as input and invokes RIDP. | ||
| class TransformPersonToXmlRequest |
There was a problem hiding this comment.
The class description mentions invoking RIDP, but the code does not seem to reflect any direct interaction with RIDP. Ensure that the comment accurately describes the class's purpose or update it to reflect the current functionality.
| def parse_json(json_string) | ||
| parsing_result = Try do | ||
| JSON.parse(json_string, :symbolize_names => true) | ||
| end | ||
| parsing_result.or do | ||
| Failure(:invalid_json) | ||
| end |
There was a problem hiding this comment.
The parse_json method uses a Try block to parse the JSON string, which is good for error handling. However, the failure case only returns a symbol :invalid_json. Consider providing more context or logging for the failure to aid in debugging.
| # @return [Dry::Monads::Result] | ||
| def call(xml_response) | ||
| parsed_xml = yield process_xml(xml_response) | ||
| # we should move all the construction into aca_entities rather than having it in FDSH |
There was a problem hiding this comment.
Comments within the code indicate future refactoring plans. It's best practice to track these outside the code, such as in a project management tool, to keep the codebase clean.
- # we should move all the construction into aca_entities rather than having it in FDSH
- # we should move all the validation into aca_entities rather than having it in FDSHAlso applies to: 24-24, 39-39
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.
| # we should move all the construction into aca_entities rather than having it in FDSH |
There was a problem hiding this comment.
This PR is still a draft noting future work to do, should be left as is for now
There was a problem hiding this comment.
@kristinmerbach Understood, the comments will remain as notes for future work while the PR is in draft status. Thank you for the clarification.
If you found the review helpful, we would love if you give us a shout-out on X (twitter).
| # rubocop:disable Metrics/MethodLength | ||
| def get_individual_response_set(individual_response_set) | ||
| { | ||
| CaseNumber: individual_response_set&.CaseNumber, | ||
| NonCitLastName: individual_response_set&.NonCitLastName, | ||
| NonCitFirstName: individual_response_set&.NonCitFirstName, | ||
| NonCitMiddleName: individual_response_set&.NonCitMiddleName, | ||
| NonCitBirthDate: individual_response_set&.NonCitBirthDate, | ||
| NonCitEntryDate: individual_response_set&.NonCitEntryDate, | ||
| AdmittedToDate: individual_response_set&.AdmittedToDate, | ||
| AdmittedToText: individual_response_set&.AdmittedToText, | ||
| NonCitCountryBirthCd: individual_response_set&.NonCitCountryBirthCd, | ||
| NonCitCountryCitCd: individual_response_set&.NonCitCountryCitCd, | ||
| NonCitCoaCode: individual_response_set&.NonCitCoaCode, | ||
| NonCitProvOfLaw: individual_response_set&.NonCitProvOfLaw, | ||
| NonCitEadsExpireDate: individual_response_set&.NonCitEadsExpireDate, | ||
| EligStatementCd: individual_response_set&.EligStatementCd, | ||
| EligStatementTxt: individual_response_set&.EligStatementTxt, | ||
| IAVTypeCode: individual_response_set&.IAVTypeCode, | ||
| IAVTypeTxt: individual_response_set&.IAVTypeTxt, | ||
| WebServSftwrVer: individual_response_set&.WebServSftwrVer, | ||
| GrantDate: individual_response_set&.GrantDate, | ||
| GrantDateReasonCd: individual_response_set&.GrantDateReasonCd, | ||
| SponsorDataFoundIndicator: individual_response_set&.SponsorDataFoundIndicator, | ||
| ArrayOfSponsorshipData: construct_sponsorship_data(individual_response_set&.ArrayOfSponsorshipData), | ||
| SponsorshipReasonCd: individual_response_set&.SponsorshipReasonCd, | ||
| AgencyAction: individual_response_set&.AgencyAction, | ||
| FiveYearBarApplyCode: individual_response_set&.FiveYearBarApplyCode, | ||
| QualifiedNonCitizenCode: individual_response_set&.QualifiedNonCitizenCode, | ||
| FiveYearBarMetCode: individual_response_set&.FiveYearBarMetCode, | ||
| USCitizenCode: individual_response_set&.USCitizenCode | ||
| } | ||
| end | ||
| # rubocop:enable Metrics/MethodLength |
There was a problem hiding this comment.
The methods are quite long, which is acknowledged by disabling Rubocop's Metrics/MethodLength. Consider refactoring to reduce method complexity and improve maintainability.
app/operations/fdsh/vlp/rx92/process_initial_verification_response.rb
Outdated
Show resolved
Hide resolved
app/operations/fdsh/vlp/rx92/handle_initial_verification_request.rb
Outdated
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Enhancements
Refactor