Add invalid announcements error widget and add related test (#2568)#5250
Add invalid announcements error widget and add related test (#2568)#5250bstepanovski wants to merge 3 commits intomasterfrom
Conversation
Bubballoo3
left a comment
Bubballoo3
left a comment
There was a problem hiding this comment.
The model updates here look good, but we are handling a bit too much logic in the view side. By checking validity in the controller and raising any errors from there, we will save a lot of code and headache in the views.
It looks like some of the other announcement tests are failing, so you will probably want to add an announcement to your dev dashboard to test it out manually and see what's going on. If you follow the trace of where the announcement path comes from, you'll see in UserConfiguration#announcement_path that it will read from an OOD_ANNOUNCEMENT_PATH environment variable before looking in the default locations. So you should be able to set this in a .env.local and point it to a file in your home directory.
| @@ -0,0 +1,24 @@ | |||
| require 'test_helper' | |||
|
|
|||
| class AnnouncementParseErrorTest < ActionDispatch::IntegrationTest | |||
There was a problem hiding this comment.
We can just add this to announcement_views_test to keep things in one place
| <% end %> | ||
| </div> | ||
| <% end %> No newline at end of file | ||
| <% if announcement.parse_error? %> |
There was a problem hiding this comment.
We typically want to do error checking in the controller (ApplicationController#set_announcements in this case)
| @@ -0,0 +1,8 @@ | |||
| <div class="alert alert-danger card"> | |||
There was a problem hiding this comment.
We shouldn't have to make a new alert partial for this error, we can use a shared alert structure by calling flash.now from the controller.
github-project-automation
Bot
moved this from Awaiting Review
to Changes Requested
in PR Review Pipeline
github-project-automation
Bot
moved this from Changes Requested
to Merged/Closed
in PR Review Pipeline
github-project-automation
Bot
moved this from Merged/Closed
to Awaiting Review
in PR Review Pipeline
|
This certainly looks better and it now does everything in the right place. Unfortunately something in FilesController seems to be snagging on the error and duplicating it every time a files/fs page attempts to load (the stacking is produced by clicking a favorites link from inside a files table view). In addition to duplicating the error message, it also stops the actual files request from responding properly. I will try to dig in and help narrow down what the actual issue is, but would love to know if you are able to replicate 
Just to be clear, this should be a 'soft' error that alerts on every page but does not block any other code like it does here. |
4 participants
Fixes #2658
Also manually tested in the dashboard with both valid and invalid announcements.