Correcting Logic for archive/unarchive collection process #339
Conversation
jchavez137
requested review from
Corpratespaz,
JohnKallies,
manuel-delvillar,
schagantiwgu and
wgu-edwin
|
I'd like to see some kind of addition to an existing test that covers this case |
Per our meeting time, I'd like to see a unit test that flexes the execution paths in this case statement. It's OK to adjust the scope. Protected would be OK if your test has the right scope. |
| PublishStatus.Workspace -> { | ||
| dao.status = PublishStatus.Workspace | ||
| } | ||
| else -> {} |
There was a problem hiding this comment.
Can this case ever happen? CollectionDao.status is not nullable, and the dao argument is also not nullable.
There was a problem hiding this comment.
yeah I just supposed that this was like a default in java case statement, I have heard that it is a good practice to leave not skip the default even if nothing needs to happen there.
| @Autowired | ||
| lateinit var collectionController: CollectionController | ||
|
|
||
| var authentication: Authentication = Mockito.mock(Authentication::class.java) |
There was a problem hiding this comment.
In Kotlin, we often use mockk. I can't give you a deep reason why, but there are times/places where Mockito is problematic. I'm not asking for a change here, just please be aware.
There was a problem hiding this comment.
Yeah I know, I just replicated a code that I already had before knowing about mockk, and since Mockito was working for the needed here I just used it here.
| author = "newAuthor") | ||
| val collection = collectionRepository.create(name = "name", user = "user", email = "j.chavez@wgu.edu", description = "description") | ||
| collection!!.status = PublishStatus.Archived | ||
| collection!!.archiveDate = LocalDateTime.now() |
There was a problem hiding this comment.
IDEA reports that collection!! isn't needed on line 106
There was a problem hiding this comment.
Yeah, the assertion is already in line 105, let me remove it.
| description = "newDescription", | ||
| publishStatus = PublishStatus.Unarchived, | ||
| author = "newAuthor") | ||
| val collection = collectionRepository.create(name = "name", user = "user", email = "j.chavez@wgu.edu", description = "description") |
There was a problem hiding this comment.
Can we please not use real identities in source code?
There was a problem hiding this comment.
Correcting this..
| } | ||
|
|
||
| @Test | ||
| fun `applyStatusChange() should apply Published status`() { |
There was a problem hiding this comment.
This test name does not seem to align with the assertion? Let's straighten this out please?
Uh oh!
There was an error while loading. Please reload this page.