-
Notifications
You must be signed in to change notification settings - Fork 74
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
Show Run#metadata in the web UI #903
Conversation
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.
Rendering metadata seems absolutely reasonable to me! But I wonder if we should still try to define two separate partials for arguments & metadata so applications have the opportunity to override them independently.
We could still have the default one called _serializable
but I wonder if we should define _arguments
and _metadata
which will render _serializable
by default but allow applications to customize them independently
@adrianna-chang-shopify @etiennebarrie would love to hear your opinions!
I would prefer separate partials 👍 |
9f18867
to
f638265
Compare
Thank you for the review! I adjusted the partials based on the feedback. |
One issue I see is that we can't really assume diff --git i/test/dummy/config/application.rb w/test/dummy/config/application.rb
index 4f864e4..f5d023f 100644
--- i/test/dummy/config/application.rb
+++ w/test/dummy/config/application.rb
@@ -19,6 +19,7 @@ class Application < Rails::Application
config.to_prepare do
MaintenanceTasks.job = "CustomTaskJob"
+ MaintenanceTasks.metadata = -> { "foo" }
end
# Only include the helper module which match the name of the controller. |
Thank you for pointing it out! I didn't realize that. Because a JSON can have arbitrary structure and displaying it nicely can be tricky. Can we simply dump JSON in the |
Maybe we could support both immediate values like strings, numbers, true and false, and complex values like objects and arrays. Let's do the minimum thing like if it's a hash, show the fields separately, otherwise just an And to be confirmed, but we should see if it's easy to override the partial if needed, that way any more complex scenarios can be handled by the developers of the app by implementing the partial? |
As the example in the README, a common use case of metadata is to record who ran the task. By displaying metadata in the web UI, we can know who ran the task at a glance.
f638265
to
562aa52
Compare
Done. Could you take a look? |
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.
thank you! No concerns from my side
Let's wait for @etiennebarrie's opinion
It works great. I tried and we can indeed override the partial if needed: diff --git c/test/dummy/app/views/maintenance_tasks/runs/_metadata.html.erb i/test/dummy/app/views/maintenance_tasks/runs/_metadata.html.erb
new file mode 100644
index 0000000..8d293b6
--- /dev/null
+++ i/test/dummy/app/views/maintenance_tasks/runs/_metadata.html.erb
@@ -0,0 +1,3 @@
+<% if metadata.present? %>
+ <p>Started by <%= metadata %></p>
+<% end %>
diff --git c/test/dummy/config/application.rb i/test/dummy/config/application.rb
index 4f864e4..14291df 100644
--- c/test/dummy/config/application.rb
+++ i/test/dummy/config/application.rb
@@ -19,6 +19,7 @@ class Application < Rails::Application
config.to_prepare do
MaintenanceTasks.job = "CustomTaskJob"
+ MaintenanceTasks.metadata = -> { "Étienne Barrié" }
end
# Only include the helper module which match the name of the controller. Maybe it's worth documenting, but maybe it's better not to. We already have In any case, the feature out of the box is useful, thanks for your contribution! |
As the example in the README, a common use case of metadata is to record who ran the task. By displaying metadata in the web UI, we can know who ran the task at a glance.