Fixes #7665 - added manage command to user mail notifications #513
Conversation
yifatmakias
force-pushed
the
7665
branch
2 times, most recently
from
88a983e to
4c47da8
Compare
There was a problem hiding this comment.
Thanks, @yifatmakias for keeping this going! Commands seem to be working.
Few inline suggestions though.
| field :name, _('Name') | ||
| field :description, _('Description') | ||
| field :interval, _('Interval') | ||
| field :mail_query, _('Mail query') unless :mail_query.nil? |
There was a problem hiding this comment.
What exactly you want to achieve here?
:mail_query.nil? is always false.
There was a problem hiding this comment.
Only with one type of mail notification, there is a mail query field so I thought this way it will show the field only when it is relevant. In a second check, the column will appear all the time, if there is a mail query it will appear in the column, if not it will be empty.
long story short - I removed the unless part it was redundant.
| @@ -0,0 +1,32 @@ | |||
| require File.join(File.dirname(__FILE__), 'test_helper') | |||
|
|
|||
There was a problem hiding this comment.
❤️
I'd also prefer to write just one test per PR, but if we've got updated test data and more than one command, then it will be nice to cover them as well.
There was a problem hiding this comment.
I added more tests to all of the commands. You are definitely right there should be at least one test to each command.
There was a problem hiding this comment.
Thanks, @yifatmakias! Few more little nitpicks and I stop bothering you :D
| field :name, _('Name') | ||
| field :description, _('Description') | ||
| field :interval, _('Interval') | ||
| field :mail_query, _('Mail query') |
There was a problem hiding this comment.
Actually, if you want to hide mail_query field you can specify hide_blank option:
field :mail_query, _('Mail query'), Fields::Field, hide_blank: true. This will hide the field if value for it is nil. Although it doesn't work for table output. So, it's up to you whether have it or not.
There was a problem hiding this comment.
I don't see a reason to add this if it does not work for table output. Do you?
There was a problem hiding this comment.
Well, it works for base, csv and other outputs, which are used more for automation purposes. I don't see much usage of this option in the project anyway, so we can leave it as it is now.
There was a problem hiding this comment.
Sounds good to me :)
|
Thanks, @yifatmakias! All seems to be ready for merge, so merging this one. |
(cherry picked from commit 4bcbcd7)
This PR is blocked by another PR:
theforeman/foreman#7549