[CO-296] Extract and make ZenDesk API testable #13
Conversation
HirotoShioi
left a comment
HirotoShioi
left a comment
There was a problem hiding this comment.
I've left some comments. Generally, I'm very interested in how this changes would enable us to do simple tests!
| Nothing -> pure page0 | ||
|
|
||
| -- | Send API request to post comment | ||
| postTicketComment |
There was a problem hiding this comment.
Since we've defined ZendeskResponse in the previous PR, we can make this function to take ZendeskResponse as an argument which will be much more simpler
postTicketComment :: (MonadIO m, MonadReader Config m) => ZendeskResponse -> m ()
src/Zendesk/Types.hs
Outdated
| { zrComment :: Text | ||
| , zrTags :: [Text] -- TODO(ks): This should be wrapped | ||
| , zrIsPublic :: Bool | ||
| { zrComment :: Text |
There was a problem hiding this comment.
Can you add ! to these fields?
| let getAgentId = zlGetAgentId . cfgZendeskLayer $ cfg' | ||
| agentId <- runApp getAgentId cfg' | ||
| let cfg = cfg' { cfgAgentId = agentId } | ||
| -- At this point, the configuration is set up and there is no point in using a pure IO. |
There was a problem hiding this comment.
I'm not sure what this means (Not enough haskell knowledge to understand it). Can you explain it to me?
There was a problem hiding this comment.
It just means that there is no point in using IO all over the codebase since the Config is initialized and we require Config to use the App newtype.
The consequences of this are - try to see how many runApp were removed...
| -- | The Zendesk API interface that we want to expose. | ||
| -- We don't want anything to leak out, so we expose only the most relevant information, | ||
| -- anything relating to how it internaly works should NOT be exposed. | ||
| data ZendeskLayer m = ZendeskLayer |
There was a problem hiding this comment.
I can see what it does but I'm not sure how this would enable us to do simple tests. (Again, now enough haskell knowledge!)
There was a problem hiding this comment.
I guess you can imagine ZendeskLayer as an object (if an object is a dictionary of functions) that we pass around.
When we want to use it we use the Reader to fetch the function we are interested in.
The enables us to replace the functions anyway we want because they are defined in the Config that Reader is using.
In other words, if we now run a test and want listTickets to return an empty list, we can, because all we need to do is construct a new configuration with that function a simple constant that returns an empty list and we can test the rest of the code.
If this is confusing, the next PR might make more sense since it will deal with tests themselves.
src/Lib.hs
Outdated
| import LogAnalysis.KnowledgeCSVParser | ||
| import Util | ||
| import CLI | ||
| import Zendesk |
There was a problem hiding this comment.
Can you make these imports explicit?
HirotoShioi
left a comment
HirotoShioi
left a comment
There was a problem hiding this comment.
Checked the changes. Looks good!
https://iohk.myjetbrains.com/youtrack/issue/CO-296
A first major refactoring. I tried to pick the simplest way of introducing mocking capabilities, required for https://iohk.myjetbrains.com/youtrack/issue/CO-289