Skip to content
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

Add organisation id for related parties and add proprietary amount #141

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marac990
Copy link

@marac990 marac990 commented Mar 7, 2023

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #141 (81c9f2b) into master (211c757) will increase coverage by 0.01%.
The diff coverage is 84.61%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##             master     #141      +/-   ##
============================================
+ Coverage     83.06%   83.07%   +0.01%     
- Complexity      663      668       +5     
============================================
  Files            78       78              
  Lines          1612     1625      +13     
============================================
+ Hits           1339     1350      +11     
- Misses          273      275       +2     
Impacted Files Coverage Δ
src/Decoder/EntryTransactionDetail.php 98.38% <60.00%> (-1.07%) ⬇️
src/DTO/Creditor.php 100.00% <100.00%> (ø)
src/DTO/Debtor.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

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

OrgId seems fine, as long as the null thing is fixed.

I am less sure about the proprietary amount though. You are reading Tp, and according to spec this is:

6.1.3.1.5.1 Type
Presence: [1..1]
Definition: Specifies the type of amount.
Datatype: "Max35Text" on page 372

It does not sound like an amount of money at all. It could be pretty much any free text. What data to you see in this field ? Instead should you not read 6.1.3.1.5.2 Amount <Amt> ?

Also the proprietary amount is not covered by any tests, and this is a no-go. You should at the very least add a realistic example of data in regression tests.

src/Decoder/EntryTransactionDetail.php Outdated Show resolved Hide resolved
@marac990
Copy link
Author

Hi, regarding this I've added a real TxDtls from the statement that we receive from the bank (of course real data is changed for some dummy data). And in our case we need that parsed.

@marac990 marac990 requested a review from PowerKiKi March 20, 2023 08:44
Copy link
Collaborator

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

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

Checks on this PR are still failing. Everything need to be green, and the PR should be rebased on top of master, before I can properly review the whole thing.

@@ -26,4 +27,14 @@ public function getName(): ?string
{
return $this->name;
}

public function setOrgId(string $orgId): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

orgId must be renamed into organisationId, in the private member, setter and getter.

@orukaz
Copy link

orukaz commented Aug 2, 2023

Yes, Related Parties and ID-s are very neede, maybe I can help with that:

<RltdPties>
                            <Dbtr>
                                <Nm>Some person</Nm>
                                <Id>
                                    <PrvtId>
                                        <Othr>
                                            <Id>12345678</Id>
                                            <SchmeNm>
                                                <Cd>NIDN</Cd>
                                            </SchmeNm>
                                        </Othr>
                                    </PrvtId>
                                </Id>
                            </Dbtr>
                            
<RltdPties>
                            <Dbtr>
                                <Nm>Business Ltd</Nm>
                                <Id>
                                    <OrgId>
                                        <Othr>
                                            <Id>675675676</Id>
                                            <SchmeNm>
                                                <Cd>COID</Cd>
                                            </SchmeNm>
                                        </Othr>
                                    </OrgId>
                                </Id>
                            </Dbtr>

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.

4 participants