Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #11674 - Bookmarks for task states #133

Merged
merged 1 commit into from Sep 7, 2015

Conversation

xprazak2
Copy link
Contributor

@xprazak2 xprazak2 commented Sep 3, 2015

I've added sortable column ended_at and bookmarks that should point to potentially problematic tasks requiring user's attention.

@@ -0,0 +1,25 @@
class AddForemanTasksBookmarks < ActiveRecord::Migration
def up
Bookmark.where(:name => "pending",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recude the number of different bookmarks: this states often don't tell anything to the end user. I would suggest two bookmars:

  • running: state = running
  • failed: state = paused OR result = error OR result = warning

@xprazak2
Copy link
Contributor Author

xprazak2 commented Sep 3, 2015

Number of bookmarks reduced to 2 and file moved from migrations to seeds.d

end

def down
Bookmark.where(:controller => "foreman_tasks_tasks").map(&:destroy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would destroy user-created bookmarks too, are we ok with it?
Wouldn't #each be more suited here, since we don't want to keep the results?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The seeds are not migrations: so just moving the file to different place is not enough. I guess inspiring more form the way we seed the bookmarks in foreman is the best thing we can do https://github.com/theforeman/foreman/blob/develop/db/seeds.d/15-bookmarks.rb

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it seems and and or operations are somehow preferred over && and ||

@xprazak2
Copy link
Contributor Author

xprazak2 commented Sep 3, 2015

I changed it into the same format as we have in Foreman.


].each do |item|
next if Bookmark.find_by_name(input[:name])
next if audit_modified? Bookmark, input[:name]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/input/item/

@iNecas
Copy link
Member

iNecas commented Sep 7, 2015

Works well, merging!

iNecas added a commit that referenced this pull request Sep 7, 2015
Fixes #11674 - Bookmarks for task states
@iNecas iNecas merged commit 983c8bb into theforeman:master Sep 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants