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

[E-Document Connector] Logiq E-Document Connector #27562

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tstefanovicius
Copy link

@tstefanovicius tstefanovicius commented Oct 30, 2024

E-Documents connector interface implementation between Business Central and Logiq

Work Item(s)
Fixes #27058
Fixes AB#541809

@tstefanovicius
Copy link
Author

@microsoft-github-policy-service agree company="Companial"

@github-actions github-actions bot added the linked Issue is linked to a Azure Boards work item label Oct 30, 2024
@tstefanovicius tstefanovicius changed the title [E-Document Connector] Logiq E-Document Connector #27101 [E-Document Connector] Logiq E-Document Connector Oct 30, 2024
@tstefanovicius
Copy link
Author

New pull request instead of #27101

@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Oct 31, 2024
@Groenbech96
Copy link
Contributor

Thanks for fixing the issue.
Please move the code to EDocumentConnectors

  • Create app for the connector.
  • Create app for the test automation.

Feel free to look at Avalara as the example.

In the mean time we will start looking at the code.

Copy link
Contributor

@Groenbech96 Groenbech96 left a comment

Choose a reason for hiding this comment

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

Some initial comments.
Overall the code is good quality and i will continue review.

Please see comment about moving to seperate app :)


enum 6380 "Logiq API Engine"
{
Extensible = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark it as internal.

Copy link
Author

Choose a reason for hiding this comment

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

Marked as Internal

@@ -0,0 +1,19 @@
namespace Microsoft.EServices.EDocumentConnector.Logiq;

enum 6380 "Logiq API Engine"
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont prefix with Logiq

Copy link
Author

Choose a reason for hiding this comment

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

Removed prefix

@@ -0,0 +1,182 @@
namespace Microsoft.EServices.EDocumentConnector.Logiq;

codeunit 6380 "Logiq Auth"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the code to EDocumentConnectors

Create app for the connector.
Create app for the test automation.
Feel free to look at Avalara as the example.

In the mean time we will start looking at the code.

Copy link
Author

Choose a reason for hiding this comment

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

Created connector and test apps. Moved to EDocumentConnectors folder

ResponseMessage: HttpResponseMessage;
AccessToken, RefreshToken : SecretText;
AccessTokExpires, RefreshTokExpires : DateTime;
AuthenticationFailedErr: Label 'Logiq authentication failed. Please check the user credentials.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Labels needs to be on global var.

Copy link
Author

Choose a reason for hiding this comment

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

Labels moved to global var


internal procedure CheckUserCredentials(var LogiqConnectionUserSetup: Record "Logiq Connection User Setup")
var
NoSetupErr: Label 'No user setup found. Please fill the user setup in the Logiq Connection User Setup page.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Labels to global var

Copy link
Author

Choose a reason for hiding this comment

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

Labels moved to global var


page 6380 "Logiq Connection Setup"
{
Caption = 'E-Document External Connection Setup';
Copy link
Contributor

Choose a reason for hiding this comment

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

Different name. Use connector name

Copy link
Author

Choose a reason for hiding this comment

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

Renamed

DataClassification = CustomerContent;
fields
{
field(1; PK; Code[10])
Copy link
Contributor

Choose a reason for hiding this comment

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

20 is standard.

Copy link
Author

Choose a reason for hiding this comment

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

Changed Code 10 > 20

Added tests
Added app.json files
Fixes
@tstefanovicius tstefanovicius requested a review from a team as a code owner November 22, 2024 12:54
@Groenbech96
Copy link
Contributor

@tstefanovicius new interfaces will break the build :)

@tstefanovicius
Copy link
Author

tstefanovicius commented Nov 22, 2024

Ahh, it was committed two days ago, will update

Copy link
Contributor

@Groenbech96 Groenbech96 left a comment

Choose a reason for hiding this comment

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

Will continue to look later. Good changes so far!

@@ -0,0 +1,20 @@
namespace Microsoft.EServices.EDocumentConnector.Logiq;
Copy link
Contributor

Choose a reason for hiding this comment

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

All files should contain the license comments as the first thing.

{
addafter(GetApproval)
{
action(UpdateStatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

Great that we have an action. 👍 🥇

Set visibility based on if document relates to Service, and is incomming/Outgoing?
Can you change this to use the IDocumentAction interface. Then you dont have to do the manual logging you are doing. You should not do that essentially. Framework shoud.

I can see that this also checks approval. What is stauses is it checking. Do we have a more specific name?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is for sent documents. Can we use Approval action that already exissts?

Choose a reason for hiding this comment

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

Sent documents in Logiq system can have statuses: "received" and "processed" (which we translate to "In Progress Logiq" service status), "distributed" (translate to approved to work with the framework even thought it's not exactly the same status) and "failed" (translate to "Failed Logiq"). We could use standard approval action but if I understand correctly, the standard status logging would not recognize custom service statuses as processed or error, so the document status would always be in progress.

@Groenbech96
Copy link
Contributor

I removed Update function. You are trying to achieve something that is already natively supported, aka Sending with async confirmation. Now code uses JQ automatically to check if document was approved (distributed) in the service.

Removed the need for page ext, and enum ext, because of above.
Fixed tests to run on the JQ instead of manual actions.

@tstefanovicius
Copy link
Author

Changes look good to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integration GitHub request for Integration area linked Issue is linked to a Azure Boards work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BC Idea]: Enable connector with Logiq for E-Documents
4 participants