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

chore: P4PU-145 create feign client for biz-events service #5

Merged
merged 12 commits into from
Jun 4, 2024

Conversation

Giuseppe-LaManna
Copy link
Collaborator

List of Changes

  • Added Feign Client for BizEvents Api transactionsList
  • Added Junit test

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@Giuseppe-LaManna Giuseppe-LaManna requested a review from a team as a code owner May 30, 2024 15:11
@and-mora
Copy link
Contributor

and-mora commented May 31, 2024

according to the recent handbook and versioning this PR can be marked with the semantic commit chore since the new code is not reachable.

@and-mora
Copy link
Contributor

fix the sonar issues, please

@Giuseppe-LaManna Giuseppe-LaManna changed the title feat: P4PU-145 create feign client for biz-events service chore: P4PU-145 create feign client for biz-events service May 31, 2024
src/main/resources/application.yml Outdated Show resolved Hide resolved
import it.gov.pagopa.arc.connector.bizevents.dto.BizEventsTransactionsListDTO;

public interface BizEventsConnector {
BizEventsTransactionsListDTO transactionsList(String fiscalCode, String continuationToken, int size);
Copy link
Contributor

Choose a reason for hiding this comment

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

may you change the method name into something verbish like 'getSomething'?

Comment on lines 28 to 34
}catch (FeignException e) {
bizEventsTransactionsListDTO =
BizEventsTransactionsListDTO
.builder()
.transactions(new ArrayList<>())
.build();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we lack logging (and related observability) here.

Copy link
Contributor

Choose a reason for hiding this comment

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

plus: I don't like very much the "catch all status codes" mode. We could change to "catch the 404 or rethrow the exception", I find it more transparent as if any errors occurs we can see it.
Later we can think to catch the other status codes when we know what to do

"matches": "x_api_key([0-9]?)+"
},
"x-fiscal-code": {
"matches": "DUMMY_FISCAL_CODE_DEFAULT"
Copy link
Contributor

Choose a reason for hiding this comment

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

we might call it ERROR instead of DEFAULT

@@ -61,6 +61,9 @@ microservice-chart:
APPLICATIONINSIGHTS_PREVIEW_PROFILER_ENABLED: "false"
ENABLE_AUDIT_APPENDER: "TRUE"

externalConfigMapValues:
Copy link
Contributor

Choose a reason for hiding this comment

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

why externalConfigMapValues? do we have a config map defined in the infrastructure code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link

sonarqubecloud bot commented Jun 4, 2024

@Giuseppe-LaManna Giuseppe-LaManna merged commit e26fa0a into develop Jun 4, 2024
6 checks passed
@Giuseppe-LaManna Giuseppe-LaManna deleted the P4PU-145-creare-feign-client-per-biz-events branch June 4, 2024 14:17
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.

2 participants