-
Notifications
You must be signed in to change notification settings - Fork 106
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 dismissible and required features to announcements #3667
Conversation
Some sample announcements: msg: HELLO
required: true msg: HELLO
type: warning
dismissible: true
button_text: "Accept" id: policy_556234
msg: Policy Text
required: true
button_text: "Accept" |
Hey sorry for the delay in getting around to this. Honestly, I kinda feel like all announcements should be dismiss-able. I haven't tested this out but will do so shortly. Looking it over though, it seems feature complete. Not sure what you mean by prototype or if that was just a qualifier you had in case I'd requested more or less. In any case - it looks pretty good on the surface, I just need a sec to play around with it. |
Thanks Jeff, great that you like the new features for Announcements I need to add tests and review the implementation in case I missed an edge case. |
8c07b6a
to
02a1f2b
Compare
I have completed the refactoring and added some tests. |
apps/dashboard/config/routes.rb
Outdated
post 'settings', :to => 'settings#update' | ||
post 'settings/:action', to: 'settings#:action', as: 'settings' |
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.
I can't quite discern why we need this change. Is it because of the id change I requested?
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 change is not related to your suggestions/changes.
This is an improvement I wanted to make to have specific methods in the controller to manage the updates to different types of settings.
The idea being that updating a profile will be managed by the profile
method. Updating an announcement will be managed by the announcement
method. This way we can apply logic specific to the type without having to add a lot of conditional logic to the previous single update
method. I wanted specific logic for the announcements update to set the value of the current date to the announcement id that is being accepted.
I can revert back to the previous way with the single method update and leave this for a later date.
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.
I can revert back to the previous way with the single method update and leave this for a later date.
Yes please, I feel like that could explode quite easily.
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.
I have reverted the changes. The PR ready for a final review
350250b
to
449da7f
Compare
096424a
to
7222ea5
Compare
# Only ALLOWED_SETTINGS methods will be called | ||
setting, value = new_settings.first | ||
send("update_#{setting}", value) |
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.
Yea I feel like this is a bit like working by coincidence
(specifically new_settings.first
). This is the first PR to update the settings controller, so we're just at that spot, but I feel like it should be easier.
That is, I don't think we need different methods for different things we're updating. Or at least I hope not just because that'll explode.
Can't we follow what we had before? I guess I'm really asking if we need all these special methods or if we can't just merge the 2 hashes and save?
- filter the parameters coming into the controller
- read the current settings
- merge
- save
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.
I think we are better defining the contract of settings as we add more. The new contract means that you can only update one setting at the time using the SettingsController
. And the specific methods is to be able to add more logic into each setting into the backend. Otherwise it will have to move somewhere else.
My first implementation was as you suggested, but I had to allow to submit a hash as a value to support multiple announcements and the read_settings
method got complicated. This is the pseudocode for the that solution:
SettingsController
...
ALLOWED_SETTINGS = [:profile, { announcements: {} }].freeze
...
def read_settings(params)
{}.tap do |settings|
params&.each do |key, value|
value = value.to_h if value.is_a?(ActionController::Parameters)
settings[key] = value
end
end
end
# The button to summit the setting
<%=
button_to(
settings_path("settings[announcements][#{announcement.id}]" => '2024-07-18 12:00:00'),
method: :post,
class: %w[btn btn-warning me-md-2],
) do
'Accept'
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.
Actually, now that I look at the code, the read_settings
could be simplified to:
def read_settings(params)
params.to_h
end
My only concern is now allowing a hash as a parameter. Do you see this as an issue?
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.
I have updated the SettingsController as per your suggestion. Looking at it now, it is not as complicated as my first implementation. Let me know what you think
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.
Yea I get all that, that it'll get complicated as we go. IDK about passing a hash into from a form
, so I guess we'll have to work that out. If it's simple now and works, then I'm happy and we can sort of continue to refine it.
That said - at some point we're likely to make a page so that folks can edit their settings, so it's likely one giant form
with all possible configurations. So this is kinda why I'm pushing for simplicity, so that we can provide an edit
page someday.
963867d
to
d58ffe8
Compare
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.
Thanks!
This is a prototype PR and will require more work to complete the implementation.
Adding dismissible and required features to announcements.
Dismissible Announcements: Will add a button to the announcement. When executed, the system will store that the announcement was completed in the user settings.
Required Announcements: The same as dismissible, but will block all request to the application and only display an empty homepage with all the announcements until all required announcements have been executed.
Possbile implementation for #3634