[TSD-40] Send the message in to all requesters with no log in the ticket #17

HirotoShioi · 2018-06-12T01:45:50Z

Description

This PR will introduce responses to tickets with no logs attached.

Related issue

https://iohk.myjetbrains.com/youtrack/issue/TSD-40

HirotoShioi · 2018-06-12T02:43:20Z

Currently, I don't have a template response for "No log files". Asking carl to provide them!

ksaric

Generally very good, nice tests, a couple of minor changes and I'm not sure if we should run this before fixing that processTicket returns a single ZendeskResponse, otherwise we might send a couple of responses back to the user which will confuse him.

ksaric · 2018-06-13T10:24:50Z

src/Lib.hs


 processTicket :: TicketId -> App [ZendeskResponse]
-processTicket ticketId = do
+processTicket tid = do


tid = tId

ksaric · 2018-06-13T10:26:45Z

src/Lib.hs

    isAttachmentZip attachment = "application/zip" == aContentType attachment

+-- | Get zendesk responses
+getZendeskResponses :: [Comment] -> [Attachment] -> TicketInfo -> App [ZendeskResponse]


Isn't the mapping one TicketInfo - one ZendeskResponse? We should fix this, as noted above.

My first impression is that fixing this could be a little tricky. I'll try to fix it today and if I couldn't get it done by today, then I'll have to ask for your help

ksaric · 2018-06-13T10:29:47Z

src/Lib.hs

    isTicketBlacklisted :: TicketInfo -> Bool
    isTicketBlacklisted TicketInfo{..} = ticketId `notElem` [9377,10815]

+    isGoguenTestnetTicket :: TicketInfo -> Bool


isTicketInGoguenTestnet

ksaric · 2018-06-13T10:36:27Z

src/Lib.hs

-    ticketsFilter ticketInfo =
-        isTicketAnalyzed ticketInfo && isTicketOpen ticketInfo && isTicketBlacklisted ticketInfo
+    ticketsFilter ticketInfo = 
+        all (\predicate -> predicate ticketInfo) [ isTicketAnalyzed


A complicated way to say a simple thing. Remember that you will have to maintain this code and that doing something smart may not be the best things to do - do something simple instead.
I know it seems kind of harsh to say that, but it's important that you understand how little changes like these make the code more complex. I actually have to think about this, which is something I like to avoid as much as possible.
And if this builds, isn't all ticketInfo [ ... ] an option?

ksaric · 2018-06-13T10:48:02Z

test/Spec.hs

+                    zendeskResponses <- run appExecution
+                    assert $ not (null zendeskResponses) && not (isTaggedWithNoLogs zendeskResponses)
+
+isTaggedWithNoLogs :: [ZendeskResponse] -> Bool


Can you insert the param name? I know this is also a convention, but I have to look at the type to know what this is doing, which is not helping readability.
Try to be as explicit as possible, it helps readability and maintainability.

Sorry, I'm not sure what this means. Do you want me to fix the name of the function?

ksaric · 2018-06-13T10:51:05Z

src/Lib.hs

+getZendeskResponses :: [Comment] -> [Attachment] -> TicketInfo -> App [ZendeskResponse]
+getZendeskResponses comments attachments ticketInfo
+    | not (null attachments) = mapM (inspectAttachment ticketInfo) attachments
+    | not (null comments)    = pure <$> responseNoLogs ticketInfo


pure <$> responseNoLogs ticketInfo == responseNoLogs ticketInfo

ksaric · 2018-06-13T10:57:40Z

src/Lib.hs

    mapM_ extractEmailAddress ticketIds


 processTicket :: TicketId -> App [ZendeskResponse]


We need to fix this. We should take the last comment attachments and process the last attachment.

ksaric · 2018-06-13T10:58:36Z

test/Spec.hs

+
+                    zendeskResponses <- run appExecution
+
+                    assert $ not (null zendeskResponses) && isTaggedWithNoLogs zendeskResponses


Very nice 👍

ksaric · 2018-06-13T11:00:59Z

test/Spec.hs

+                forAll arbitrary $ \ticketInfo ->
+                    monadicIO $ do
+
+                    pre $ any (not . null . cAttachments) comments


all (not . null . cAttachments) comments - we require that all the comments have attachments, right?

Well, I thought the intended behavior was that if user had submitted multiple comments and at least one of them has log file attached(so not all comments needs log file), then processTicket should return ZendeskResponses

Yes, good thinking but I didn't even notice we were returning multiple ZendeskResponse, my bad.

HirotoShioi

Fixed most of the things, I'll try to apply the fix but it might be little tricky.

HirotoShioi · 2018-06-13T23:57:01Z

test/Spec.hs

+                forAll arbitrary $ \ticketInfo ->
+                    monadicIO $ do
+
+                    pre $ any (not . null . cAttachments) comments


Well, I thought the intended behavior was that if user had submitted multiple comments and at least one of them has log file attached(so not all comments needs log file), then processTicket should return ZendeskResponses

HirotoShioi · 2018-06-13T23:59:38Z

test/Spec.hs

+                    zendeskResponses <- run appExecution
+                    assert $ not (null zendeskResponses) && not (isTaggedWithNoLogs zendeskResponses)
+
+isTaggedWithNoLogs :: [ZendeskResponse] -> Bool


Sorry, I'm not sure what this means. Do you want me to fix the name of the function?

HirotoShioi · 2018-06-14T00:16:22Z

src/Lib.hs

    isAttachmentZip attachment = "application/zip" == aContentType attachment

+-- | Get zendesk responses
+getZendeskResponses :: [Comment] -> [Attachment] -> TicketInfo -> App [ZendeskResponse]


My first impression is that fixing this could be a little tricky. I'll try to fix it today and if I couldn't get it done by today, then I'll have to ask for your help

ksaric

Great work, just a few more little details.

ksaric · 2018-06-14T07:11:21Z

src/Lib.hs

+inspectAttachments :: TicketInfo -> [Attachment] -> App (Maybe ZendeskResponse)
+inspectAttachments ticketInfo attachments = do
+    -- Inspecting the last element since it'll be the latest attachment
+    let latestAttachment = last <$> nonEmpty attachments


How about inspectAttachment ticketInfo =<< safeHead . sort $ attachments where the Ord instance should be AttachmentId? Or the expanded form of the same which is even better.

ksaric · 2018-06-14T07:13:46Z

src/Lib.hs

+inspectAttachments ticketInfo attachments = do
+    -- Inspecting the last element since it'll be the latest attachment
+    let latestAttachment = last <$> nonEmpty attachments
+    case latestAttachment of


This should be a bind - latestAttachment >>= \att -> inspectAttachment ticketInfo att or similar.
See how they align? Nothing goes to Nothing while Just goes to Just.
You do work when it's Just otherwise return nothing.

ksaric · 2018-06-14T07:14:55Z

test/Spec.hs

+                forAll arbitrary $ \ticketInfo ->
+                    monadicIO $ do
+
+                    pre $ any (not . null . cAttachments) comments


Yes, good thinking but I didn't even notice we were returning multiple ZendeskResponse, my bad.

- Unwrap newtype so the request api will be valid - Make assignee_id field to be maybe since ticket could be unassigned in that case classifier will not be able to parse the json

HirotoShioi added 3 commits June 12, 2018 10:25

Add test cases

a9c5d87

Send the message in to all requesters with no log in the ticket

93157b4

Fix test cases

dcb7526

HirotoShioi force-pushed the hiroto/tsd-40 branch from eb91bf9 to dcb7526 Compare June 12, 2018 02:38

HirotoShioi requested a review from ksaric June 12, 2018 02:38

Create getZendeskResponses function to decompose processTicket

9cc94e7

HirotoShioi changed the title ~~[TSD 40] Send the message in to all requesters with no log in the ticket~~ [TSD-40] Send the message in to all requesters with no log in the ticket Jun 13, 2018

HirotoShioi added 2 commits June 13, 2018 15:57

Filter goguen testnet tickets

eba2a39

Add response message

f739b15

ksaric suggested changes Jun 13, 2018

View reviewed changes

Minor fixes

0fd05ab

HirotoShioi commented Jun 14, 2018

View reviewed changes

HirotoShioi added 3 commits June 14, 2018 12:38

Return single ZendeskResponse instead of list of responses

16a7cb6

Inspect only latest attachment

42d81b5

Sort comment by id, inspect last element of the attachments

494cfb3

ksaric approved these changes Jun 14, 2018

View reviewed changes

HirotoShioi and others added 6 commits June 14, 2018 16:53

Resolve merge conflict

05d234d

Fix flag

3706e4f

Minor fixes

f251294

[TSD-40] The impure version.

1530f3c

[TSD-40] The version with the pure function. Either is an option here.

9e5864b

Couple of minor fixes

4559edf

- Unwrap newtype so the request api will be valid - Make assignee_id field to be maybe since ticket could be unassigned in that case classifier will not be able to parse the json

HirotoShioi merged commit 0d57a90 into develop Jun 15, 2018

HirotoShioi deleted the hiroto/tsd-40 branch June 15, 2018 10:16

		mapM_ extractEmailAddress ticketIds


		processTicket :: TicketId -> App [ZendeskResponse]


		zendeskResponses <- run appExecution

		assert $ not (null zendeskResponses) && isTaggedWithNoLogs zendeskResponses

[TSD-40] Send the message in to all requesters with no log in the ticket #17

[TSD-40] Send the message in to all requesters with no log in the ticket #17

Uh oh!

Conversation

HirotoShioi commented Jun 12, 2018

Description

Related issue

Uh oh!

HirotoShioi commented Jun 12, 2018

Uh oh!

ksaric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirotoShioi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaric left a comment

Choose a reason for hiding this comment

Uh oh!

ksaric Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ksaric Jun 14, 2018 •

edited

Loading