APS-1744 bug - fix multiple beds in room issue with excel seed job #2728
Conversation
danhumphreys-moj
changed the title
danhumphreys-moj
force-pushed
the
bug/fix-multiple-beds-in-room-issue-with-Excel-seed-job
branch
from
296a144 to
58d0839
Compare
| ) : ExcelSeedJob { | ||
| private val log = LoggerFactory.getLogger(this::class.java) | ||
| private val questionCriteriaMapping = QuestionCriteriaMapping(characteristicRepository) |
There was a problem hiding this comment.
if we want this to be newly constructed on every run of the seed job this will need to be constructed when the seed job is executed e.g. in processDataFrame, and then 'passed around' to functions that need it
There was a problem hiding this comment.
Will revisit this when work is done to address APS-1757.
| ) : ExcelSeedJob { | ||
| private val log = LoggerFactory.getLogger(this::class.java) | ||
| private val questionCriteriaMapping = QuestionCriteriaMapping(characteristicRepository) | ||
| private lateinit var qCode: String |
There was a problem hiding this comment.
this shouldn't be a class instance variable to avoid concurrency issues
There was a problem hiding this comment.
Will revisit this when work is done to address APS-1757.
| @@ -83,13 +87,13 @@ class ApprovedPremisesRoomsSeedFromXLSXJob( | |||
| val beds = mutableListOf<BedEntity>() | |||
| for (i in 1..<dataFrame.columnsCount()) { | |||
| val roomAnswers = dataFrame.getColumn(i) | |||
| val bedNumber = roomAnswers[1].toString() | |||
| val roomCode = "$qCode - ${roomAnswers[0]}" | |||
There was a problem hiding this comment.
may be worth introducing a fun buildRoomCode(qCode,roomNumber) = "$qCode - $roomNumber" to reduce duplication
There was a problem hiding this comment.
Will revisit this when work is done to address APS-1757.
| "1", | ||
| "2", | ||
| ) | ||
| rows.addCharacteristics(3, mapOf("Is this room located on the ground floor?" to listOf(1, 2))) |
There was a problem hiding this comment.
it would be really helpful/more readable if we added arg names when using addCharacteristics
There was a problem hiding this comment.
Would this be more readable like:
rows.addCharacteristics("Is this room located on the ground floor?", listOf("N","Y","Y"))
There was a problem hiding this comment.
Will revisit this when work is done to address APS-1757.
|
|
||
| fun MutableList<String>.addCharacteristics(numberOfRooms: Int = 1, activeCharacteristics: Map<String, List<Int>> = emptyMap()) { | ||
| siteSurvey.questionToCharacterEntityMapping.keys.forEach { question -> | ||
| questionCriteriaMapping.questionToCharacterEntityMapping.keys.forEach { question -> |
There was a problem hiding this comment.
as discussed, propose copying all questions into a List in this class as an instance variable, and then referring to that here instead of questionCriteriaMapping.questionToCharacterEntityMapping.keys
There was a problem hiding this comment.
Will revisit this when work is done to address APS-1757.
danhumphreys-moj
force-pushed
the
bug/fix-multiple-beds-in-room-issue-with-Excel-seed-job
branch
from
19cc622 to
5dc6481
Compare
danhumphreys-moj
deleted the
bug/fix-multiple-beds-in-room-issue-with-Excel-seed-job
branch
No description provided.