Fix rerendering on invalid form.yml.erb files - #5319
andrejcermak wants to merge 4 commits into
Conversation
|
Even though this is an individual PR, it could also be seen as part of the caching initiative as it caches invalid apps and doesn't allow re-rendering. |
|
The other PR you have open may be eaiser to merge because it's simpler. This, I'm not so sure about. It'll require some thought, which we don't have a lot to spare given we're trying to merge all our accessibility issues and urgently release 4.2 for the same. |
|
@johrstrom thats fine by me, I think the other PR was much more important. This is more of a failsafe mechanism and is not urgent. |
There was a problem hiding this comment.
Thanks for your patience Andrej, this seems to be fairly straightforward and aside from a few comments/questions I think it looks pretty good. The tests indicate that this changes some of the error reporting, which we should try to avoid if possible. Just FYI, I recently added a testing section https://github.com/OSC/ondemand/blob/master/DEVELOPMENT.md#testing-the-dashboard to our development docs which you can use to run and debug the failing tests.
While you're at it, it would also be nice to include a test case that demonstrates the original re-rendering issue and shows that it no longer occurs. Thanks!
| # Check if a batch connect app is valid and safe to use | ||
| # Caches the validity check to avoid re-parsing form.yml.erb on every call | ||
| def valid_batch_connect_app?(app) | ||
| return false unless app.batch_connect_app? |
There was a problem hiding this comment.
I am having trouble grasping why this line (and by extension, this whole method) is necessary, given that the batch_connect_app? check is already happening on line 28 above with
(@sys_apps + @dev_apps + @usr_apps).select(&:batch_connect_app?).each do |ood_app|
Given that the first check is unnecessary, why not just add the rescue block to valid? and remove the helper method?
| </span> | ||
| <% end %> | ||
| <% all_settings.sort.each_with_index do | (app_token, app_saved_settings), index| | ||
| <% valid_settings = all_settings.select { |app_token, _| BatchConnect::App.from_token(app_token.to_s).valid? } %> |
There was a problem hiding this comment.
I think I would appreciate if we named the unused variable instead of a plain _. Something like _content (I am not sure what the actual value is here) would be clearer to read IMO.
| # Hash describing the full form object | ||
| def form_config(binding: nil) | ||
| return @form_config if @form_config | ||
| return @form_config if defined?(@form_config) |
There was a problem hiding this comment.
I believe that !!@form_config == defined(@form_config) unless @form_config is explicitly set to nil/false. Is this change handling an actual case where invalid ERB causes hsh.deep_merge read_yaml_erb(path: file, binding: binding) to be nil, leading to re-evaluation, or is this just a defensive precaution?
github-project-automation
Bot
moved this from Awaiting Review
to Changes Requested
in PR Review Pipeline
github-project-automation
Bot
moved this from Blocked
to Merged/Closed
in PR Review Pipeline
github-project-automation
Bot
moved this from Merged/Closed
to Awaiting Review
in PR Review Pipeline
4 participants
This PR aims to fix #5057.
Fix memoisation & UI filtering for malformed Batch Connect apps
defined?(@form_config)so an empty hash is cached and the ERB is not reparsed on every call.valid?to avoid repeated form_config look‑ups.valid_batch_connect_app?validity check