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.

Sign up for GitHub

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

Jump to bottom

MARP-616-A-Trust-Refactoring #38

Merged
merged 6 commits into from
Aug 30, 2024
Merged

MARP-616-A-Trust-Refactoring #38

merged 6 commits into from
Aug 30, 2024

Conversation

nqhoan-axonivy
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the enhancement New feature or request label Aug 22, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 22, 2024 •
edited
Loading

Test Results

5 tests  +2   5 ✅ +5   20s ⏱️ +13s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌  - 3 

Results for commit 596ff19. ± Comparison against base commit 881d7a8.

This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both. 
com.axonivy.connector.atrust.test.ATrustProcessTest ‑ callStartSignatureProcessFailed(BpmClient)

com.axonivy.connector.atrust.test.ATrustProcessTest ‑ addNewTemplateProcess(BpmClient)
com.axonivy.connector.atrust.test.ATrustProcessTest ‑ callStartSignatureFailedByInvalidParams(BpmClient)
com.axonivy.connector.atrust.test.ATrustProcessTest ‑ deleteTemplateProcess(BpmClient)

♻️ This comment has been updated with latest results.

@phhung-axonivy phhung-axonivy self-requested a review August 23, 2024 02:42
ivy-rew
ivy-rew reviewed Aug 23, 2024
View reviewed changes
a-trust-connector/src/resources/config/ATrust-OpenAPI.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

this is absolutely great, that you craft the openAPI for a-trust, this increases the maintainability of this service a lot ❤️

ivy-rew
ivy-rew reviewed Aug 23, 2024
View reviewed changes
Copy link
Member

@ivy-rew ivy-rew left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You completely had to switch the stack form SOAP to REST.
Now I do not see any tests that use mocks of the service payloads going to an fro.
Just like in the previous version it tests against live instances. We should avoid this and use a mock like in all other REST connectors. This will help us a lot:

  • presales can show a demo when the service is not reachable or they are being offline
  • we have documentation how the service worked in the past; if the service changes.

You may copy the 'mocking' approach from another connector or our examples: https://github.com/axonivy/project-build-examples/tree/master/compile-test/crmTests/src/ch/ivyteam/crm/mocks

phhung-axonivy
phhung-axonivy requested changes Aug 27, 2024
View reviewed changes
...ctor-demo/src_hd/com/axonivy/connector/atrust/demo/DocumentSignature/DocumentSignature.xhtml Outdated Show resolved Hide resolved
...ctor-demo/src_hd/com/axonivy/connector/atrust/demo/DocumentSignature/DocumentSignature.xhtml Show resolved Hide resolved
a-trust-connector-product/README_DE.md
- Unterstützt dich mit einer Demo-Implementierung, um deinen Integrationsaufwand zu reduzieren.
- Ermöglicht es low code citizen developers bestehende Geschäftsprozesse mit Handy-Signaturfunktionen zu erweitern.

Für die Nutzung von A-Trust benötigst Du einen Account, den Dir https://www.a-trust.at/de/produkte/qualifizierte_signaturservices/xidentity/ erstellt.
Copy link

Choose a reason for hiding this comment

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

It does not match the English version.

a-trust-connector/processes/ATrust/StartSignature.p.json Outdated Show resolved Hide resolved
a-trust-connector/src/com/axonivy/connector/atrust/constant/Constants.java Outdated Show resolved Hide resolved
@nqhoan-axonivy nqhoan-axonivy force-pushed the feature/MARP-616-A-Trust-Refactoring branch from 5913be0 to f334ad2 Compare August 27, 2024 11:01
@nqhoan-axonivy
Copy link
Contributor Author

Thank @ivy-rew so much for your hints!
I'm working on it now. Just make everything back to work, now I will create a mock service for testing.

@nqhoan-axonivy
Introduce new Rest Client
692e84e 
Adapt code
Adapt test
Adapt docs
@nqhoan-axonivy nqhoan-axonivy force-pushed the feature/MARP-616-A-Trust-Refactoring branch from 88f2ed8 to 692e84e Compare August 27, 2024 12:07
phhung-axonivy
phhung-axonivy approved these changes Aug 29, 2024
View reviewed changes
Copy link

@phhung-axonivy phhung-axonivy left a comment

Choose a reason for hiding this comment

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

LGTM

@nqhoan-axonivy nqhoan-axonivy merged commit 8917613 into master Aug 30, 2024
3 checks passed
@nqhoan-axonivy nqhoan-axonivy deleted the feature/MARP-616-A-Trust-Refactoring branch August 30, 2024 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivy-rew ivy-rew ivy-rew left review comments

@phhung-axonivy phhung-axonivy phhung-axonivy approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nqhoan-axonivy @ivy-rew @phhung-axonivy