-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
Partly fixes: #915 |
We should introduce a new type class to transform values to log message parts and hide sensual information from. I would do that in a different pull request. The snippet below shows a debug level log:
|
Conflicts: src/Frontend/Page/Admin.hs
this message was turning every log message redundantly into twa. we should just make sure that all msg daemon clients do enough debug logging. i know it's not exactly the same, as this doesn't show us the message queue delivered, but i think it's close enough, and the change is justified by the reduced noise.
deleteUser = update . DeactivateUser | ||
deleteUser :: (ActionSessionLog m, ActionPersist m) => AUID User -> m () | ||
deleteUser uid = do | ||
logEvent INFO $ cshow uid <> " is deactivated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sounds like the message i would expect if the user has been deactivated befre, and this action can't do it any more and is warning about it. deactivating user
is less confusing. i'll change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or, even better, why don't we just show
the acid-state transaction type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, i guess because we have the transaction log for that. (-:
@@ -78,6 +78,9 @@ type CSI' s a = CSI s s a a | |||
csi :: CSI s t a b => Iso s t a b | |||
csi = iso cs cs | |||
|
|||
cshow :: (Show a, ConvertibleStrings String c) => a -> c | |||
cshow = cs . show |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we eliminated this a while back. Why do you like names so much? (-:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anyway, we can keep it for a while. (-:
Lots of noise from me (sorry), but LGTM. This is a big improvement over the current status, thanks! @andorp can be merged if you and travis are green. |
…omplexity)." This reverts commit b23a76e. The reason the name introduction is valuable is because it de-duplicates something that is almost, but not quite, trivial, and it's good if it can only change in one place, *if* it ever has to change. See next commit for evidence in support of this point.
LGTM |
No description provided.