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

[MODDATAIMP-931] DITF forward merge #298

Merged
merged 32 commits into from
Oct 10, 2023
Merged

[MODDATAIMP-931] DITF forward merge #298

merged 32 commits into from
Oct 10, 2023

Conversation

ncovercash
Copy link
Member

@ncovercash ncovercash commented Sep 28, 2023

Jira MODDATAIMP-931

To be merged with folio-org/mod-source-record-manager#809

TODOS and Open Questions

  • RMB removed the language trait in v33.2 back in 2021 (via FOLIO-3351: Remove lang query parameter and language.raml trait raml#141). My working suspicion as to why this had not affected mod-DI yet is that data-import-raml-storage's version of raml-util was behind, and that mod-DI's version of data-import-raml-storage was even further behind than that, causing significant lag.
    • To use the newest version of data-import-raml-storage, which includes our added schemas, we must update our version.
    • The current fix is just to remove the lang param (never used) and add the new totalRecords field which, per the docs, should be sufficient.
    • We should probably open a ticket to ensure totalRecords eventually gets implemented as expected
  • I'd like to update the tests of DataImportQueueItemDaoImpl to use a real DB instead of extensive mocking. This would help catch errors in schemas, get a bit more coverage, and be easier to maintain.
    • This was not done previously as, when we wrote the DAO tests, we were unable to get liquibase running in the test environment. This has been resolved.
  • We can probably delete ModTenantApiTest — it's queries to /_/tenant and responses are tested as part of all other integration tests in AbstractRestTest
  • Investigate and potentially implement the new refresh token API for system user login (deprecation plan)
  • Use Java 17 patterns wherever applicable
    • .toList() on collectors
    • arrow switches
  • Bump version number(s)?
  • Bump interface version(s)?
  • Bump other Maven dependencies?
  • Cleanup .map(v -> someConcreteValue) calls to eliminate extra invocation overhead

Notes

I do not believe the code smell Replace this "switch" statement by "if" statements to increase readability needs to be addressed:
image

The switch statement here provides an easily readable and testable method to only cancel jobs which do not match those four statuses. An equivalent if would require a long and redundant boolean expression.

@ncovercash ncovercash changed the title [DO NOT MERGE] DITF work [MODDATAIMP-931] [DO NOT MERGE] DITF forward merge Sep 28, 2023
@ncovercash ncovercash changed the title [MODDATAIMP-931] [DO NOT MERGE] DITF forward merge [MODDATAIMP-931] DITF forward merge Oct 9, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

99.7% 99.7% Coverage
0.0% 0.0% Duplication

@ncovercash ncovercash marked this pull request as ready for review October 9, 2023 15:31
@ncovercash ncovercash requested review from TarasSpashchenko and a team October 9, 2023 16:45
@ncovercash ncovercash merged commit f48cf0b into master Oct 10, 2023
6 checks passed
@KaterynaSenchenko KaterynaSenchenko deleted the ditf-poppy branch November 8, 2023 12:56
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.

3 participants