Added dismissible and required features to announcements

[abujeda]

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

[abujeda]

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"

[johrstrom]

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.

[abujeda]

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.
It should not change the general approach. I will work on that to complete the PR

[abujeda]

I have completed the refactoring and added some tests.

[johrstrom]

I can't quite discern why we need this change. Is it because of the id change I requested?

[abujeda]

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.

[johrstrom]

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.

[abujeda]

I have reverted the changes. The PR ready for a final review

[johrstrom]

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

[abujeda]

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
  %>

[abujeda]

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?

[abujeda]

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

[johrstrom]

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.

[johrstrom]

Thanks!