-
Notifications
You must be signed in to change notification settings - Fork 108
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
Added support to render widgets partial without any layout furniture #3989
base: master
Are you sure you want to change the base?
Conversation
This look OK - I just have to take a second to consider the security of it. |
apps/dashboard/config/routes.rb
Outdated
@@ -106,6 +106,8 @@ | |||
|
|||
post 'settings', :to => 'settings#update' | |||
|
|||
match '/widgets/*widget_path', to: 'widgets#show', via: [:get, :post], as: 'widgets' |
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.
Sorry for the delay on this. Why do we need post
here?
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.
This is for completeness. There might be complex widgets that might require to submit data from the frontend to the backend for an update of the state/data of the widget after the initial render. Adding a POST will provide a more REST friendly way of getting the data.
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.
This is for completeness...
We don't seem to be passing the body or query params to the partial. That seems super risky to do so, because we don't know them up front and so can't filter them. I have to admit that scares me a bit.
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.
Well, there is no need to pass them and this is not adding something that is not already possible. The body and query params are available in any template through the params
object.
This is a valid and working widget template (tested locally to confirm)
/pun/sys/dashboard?parameter=Aday
# /pun/sys/dashboard?parameter=Aday
# widget template
<%
value = params[:parameter]
%>
<%= "Query Param Value: #{value}" %>
Doing this with a POST will be more REST friendly
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.
To mitigate the security concerns, we could make this a experimental feature for now to give us time to try and discover any issues.
# Experimental Feature
# Allows widget partials to be rendered without any page furniture.
# It can be use to extend OOD functionality.
if Configuration.widget_partials_enabled?
match '/widgets/*widget_path', to: 'widgets#show', via: [:get, :post], as: 'widgets'
end
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.
To mitigate the security concerns, we could make this a experimental feature
Yes please. I just tagged 4.0.0 but this can make it into 4.0.1.
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.
Changes completed
Added support to render widgets partials without any layout furniture.
This change will allow for better support for OOD extensions in the form of widgets and the support of dynamic content with Javascript with Widgets.
Fixes #3986