Conversation
Added new Approval APIs and Tests Related work items: microsoft#29223
| { | ||
| Caption = 'Approval Code'; | ||
| } | ||
| field(salespersPurchCode; Rec."Salespers./Purch. Code") |
There was a problem hiding this comment.
We do not use abbreviations for field names
|
|
||
| page 30094 "APIV2 - Approval Entries" | ||
| { | ||
| APIGroup = 'auditing'; |
There was a problem hiding this comment.
If these are going to be a separate group, then they shouldn't be in APIV2 app. Instead they should get their own app.
| { | ||
| APIGroup = 'auditing'; | ||
| APIPublisher = 'microsoft'; | ||
| APIVersion = 'v2.0'; |
There was a problem hiding this comment.
microsoft/auditing is a new API path, why v2.0?
- Either no new path and part of standard APIs (not favorable)
- Or new app (favorable)
Co-authored-by: Onat Buyukakkus <55088871+onbuyuka@users.noreply.github.com>
Co-authored-by: Onat Buyukakkus <55088871+onbuyuka@users.noreply.github.com>
| { | ||
| Caption = 'Substitute'; | ||
| } | ||
| field(eMail; Rec."E-Mail") |
| { | ||
| Caption = 'Sequence No.'; | ||
| } | ||
| field(senderID; Rec."Sender ID") |
There was a problem hiding this comment.
ID is not proper, it should be Id to align with other APIs
There was a problem hiding this comment.
Both field name and caption should be updated
| { | ||
| Caption = 'Sequence No.'; | ||
| } | ||
| field(senderID; Rec."Sender ID") |
There was a problem hiding this comment.
Both field name and caption should be updated
| { | ||
| Caption = 'Pending Approvals'; | ||
| } | ||
| field(recordID; Rec."Record ID to Approve") |
There was a problem hiding this comment.
How is the recordId going to be used? What is the flow in using/exposing this?
RecordId is very BC specific. We usually add the System Id and Table Id, however there should be a way of fetching the record itself.
| { | ||
| Caption = 'Posted Record ID'; | ||
| } | ||
| field(delegationDateFormula; Rec."Delegation Date Formula") |
There was a problem hiding this comment.
Date formula is very BC specific. How is it supposed to be used by the consumer of the API?
|
|
||
| // [THEN] entry should exist in the response | ||
| if GetLastErrorText() <> '' then | ||
| Assert.ExpectedError('Request failed with error: ' + GetLastErrorText()); |
There was a problem hiding this comment.
I believe this one should be Assert.Error, not expected error
| if GetLastErrorText() <> '' then | ||
| Assert.ExpectedError('Request failed with error: ' + GetLastErrorText()); | ||
|
|
||
| Assert.IsTrue(LibraryGraphMgt.GetObjectIDFromJSON(ResponseText, 'id', ApprovalEntryId), 'Could not find approval entry'); |
There was a problem hiding this comment.
Tests are weak. Just getting an ID means nothing. We should get the proper E2E scenarios that outline the usage of the API, so we understand how the API should be used.
| if GetLastErrorText() <> '' then | ||
| Assert.ExpectedError('Request failed with error: ' + GetLastErrorText()); | ||
|
|
||
| Assert.IsTrue(LibraryGraphMgt.GetObjectIDFromJSON(ResponseText, 'id', ApprovalEntryId), 'Could not find approval entry'); |
There was a problem hiding this comment.
Assert should check the actual contents of the response, checking if ID exists proves nothing.
5 participants
Summary
Added the following APIs:
Read-only API that allows to query Workflow
Read-only API that allows to query (Workflow, User) mappings
Read-only API that allows to query (Workflow, User, action) history
Depends on Objects in Base App PR https://github.com/microsoft/BusinessCentralApps/pull/1733
Work Item(s)
Fixes #29777
AB#613827