Task #2421
closedCreate general CSV parser and refactor existing import LRTs to use it
100%
Description
The goal is to create an abstract class for parsing CSV files from which every importing class parsing CSV files in extras will inherit. The existing LRTs will be rewritten to use it.
Updated by Tomáš Doischer over 4 years ago
- Status changed from New to Needs feedback
- Assignee changed from Tomáš Doischer to Peter Štrunc
- % Done changed from 0 to 80
The implementation is in branch https://github.com/bcvsolutions/czechidm-extras/tree/2421-implement-general-parser which was based on 2.5.0-RC and rkubica/2329-ImportCSVUserContractRolesTaskExecutor to make sure I have the current code for all import tasks since the refactoring was quite major.
I have done the following:- created AbstractCsvImportTask.java and AbstractCsvImportTaskTest.java as the classes from which every import task and its test inherits
- I deprecated ImportAutomaticRoleAttributesFromCSVExecutor.java since it is replaced by ImportAutomaticRoleByAttributesCSVExecutor.java which is much more useful. Some clients might use it so I don't want to remove it completely just yet. But this is up to you.
- I refactored every other import task to use the new code but functionality did not change except for a few LRTs (see below). I added localization to every task, though, because AbstractCsvImportTask.java excepts to use it.
- I refactored ImportRolesFromCSVExecutor.java to use the dynamic columns approach with prefixes for system and EAV attributes.
- I refactored ImportCSVUserContractRolesTaskExecutor.java to allow using more than one contract EAV to find the contract to which roles are added.
All tests passed. Documentation was updated.
@sourek, can you please give me feedback?
Updated by Peter Štrunc over 4 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Peter Štrunc to Tomáš Doischer
Hi, thanks for the refactoring. It finally looks nice. I have not found any major issue, just a few cosmetics:
- try not to nest try-catch blocks (AbstractCsvImportTask) - either merge try blocks (add second catch block), or extract nested try-catch to separate method
- ImportCSVUserContractRolesTaskExecutor#handleRecords needs to be refactored
- ImportCSVUserContractRolesTaskExecutor:401 Use CONTRACT_DEFINITION inbstead of string default
- ImportCSVUserContractRolesTaskExecutor:299 no need to pass definition to message if its null
- Error in tests:
[ERROR] Failures: [ERROR] ImportCSVUserContractRolesTaskExecutorTest.assignRolesToAllContractTest:184->AbstractCsvImportTaskTest.createAttachment:109 [ERROR] ImportCSVUserContractRolesTaskExecutorTest.assignRolesToPrimeContractTest:116->AbstractCsvImportTaskTest.createAttachment:109
- ImportAutomaticRoleAttributesFromCSVExecutor is not deprecated - and please add this information to locales so that it is visible in UI
- Few sonar errors here and there
Updated by Tomáš Doischer over 4 years ago
Thank you very much for your feedback, @sourek.
I implemented each suggestion. The tests did not pass because the required CSV files were not in the repo, sorry!
Can you look at it again please?
Updated by Peter Štrunc over 4 years ago
- Status changed from In Progress to Closed
- % Done changed from 80 to 100
Thanks for fixes. It looks great.