Allowing Incedent log visible to staff/public or editors #10372
Conversation
…elect new viewing permisions for incident logs (can only be modified by wca members/admin) and used enum to keep track who is able to view what log
…tion.org into incedent_log_visible_to_staff_only
| @@ -38,4 +38,4 @@ | |||
|
|
|||
| #wrc-data { | |||
| @extend #competition-data; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Here it seems you randomly removed a newline feed at the end of the file...?
| elsif current_user&.can_view_delegate_matters? # Staff see staff + public | ||
| base_model.staff_visible |
There was a problem hiding this comment.
The name of the scope (aka staff_visible) implies that "if you're Staff in any capacity, you should be able to see this incident". The check you're performing however is whether the current user can access Delegate Matters, which is distinct from being staff.
For example, Software Team members are staff but they cannot see Delegate matters. Same goes for WSOT or WMCT for example.
So either you change the check to something that relates to the user being staff, or you should rename the visibility scope for the incidents to something like delegate_visible (name tentative, feel free to come up with something better)
| unless @incident.resolved? | ||
| redirect_to_root_unless_user(:can_manage_incidents?) | ||
| end |
There was a problem hiding this comment.
This existing check should probably be "collapsed" into the two new if-checks -- that is, the visibility flag should be the indicator for access now.
(I'm assuming that once an incident is resolved, its visibility is set to public accordingly)
| if @incident.update(incident_params) | ||
| flash[:success] = "Incident was successfully updated." | ||
| redirect_to @incident | ||
| else | ||
| render :edit | ||
| end |
There was a problem hiding this comment.
Why are you removing this logic? This would make it so that incidents don't get persisted to the database :O
| { incident_competitions_attributes: [:id, :competition_id, :comments, :_destroy] }, | ||
| ] | ||
|
|
||
| params.require(:incident).permit(permitted_params) |
There was a problem hiding this comment.
Why are you refactoring this into a separate new line, instead of "just" adding :visibility to the existing parameter list?
| scope :publicly_visible, -> { where(visibility: :visible) } | ||
| scope :staff_visible, -> { where(visibility: [:staff, :visible]) } | ||
| scope :wrc_visible, -> { all } |
There was a problem hiding this comment.
Rails enum gives you scopes by default that you can reuse here. Read more at https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Enum.html
| scope :publicly_visible, -> { where(visibility: :visible) } # Public sees only visible | ||
| scope :staff_visible, -> { where(visibility: [:staff, :visible]) } # Staff sees staff + visible | ||
| scope :wrc_visible, -> { all } # WRC sees everything |
There was a problem hiding this comment.
This is (accidental?) copy-pasta from above
| def visible_to?(user) | ||
| case visibility | ||
| when "visible" | ||
| true # Everyone can see public incidents | ||
| when "staff" | ||
| user&.staff? || user&.can_manage_incidents? # Staff and WRC can see staff incidents | ||
| when "draft" | ||
| user&.can_manage_incidents? # Only WRC can see drafts | ||
| end | ||
| end |
There was a problem hiding this comment.
Interestingly, the checks you're doing here are different from the controller (see my other, longer comment about can_view_delegate_matters) but I feel like they should be the same.
Perhaps a common check method can be reused both here and in the controller?
| <%= f.select :visibility, | ||
| Incident.visibilities.map { |k, _| [k.titleize, k] }, | ||
| { include_blank: false }, | ||
| { class: 'ui dropdown', | ||
| style: 'min-width: 80px; max-height: 40px;' } %> |
There was a problem hiding this comment.
I feel like simple_form should be able to detect enums by itself? Feel free to consult the documentation: https://github.com/heartcombo/simple_form or ChatGPT (about simple_form specifically)
| { selected: @incident.visibility }, | ||
| { class: 'ui dropdown', | ||
| style: 'min-width: 80px; max-height: 40px;', | ||
| onchange: 'this.form.submit()' } %> |
There was a problem hiding this comment.
Why is this onchange listener necessary? At first glance, it doesn't feel correct to say "This form should be submitted every time a new dropdown value is selected"
#10316 Added a drop-down menu to each incident log, allowing you to set its permissions to be either public (all can see), draft (WCA members and those with permission to edit the log can see), or staff (only staff can see).