Redmine

It makes sense to me.

I'm adding also Jens and Holger from Plan.io who contributed with this feature.

I just had a look at this and I generally like it. However the patch is malformed (patch: **** malformed patch at line 393: @ -87,8 +97,127 @ class WebhooksControllerTest < Redmine::ControllerTest), I made it work with git apply --recount --3way. Also conflicts in fr.yml due to recent upstream changes. Might also split out non-en i18n into a separate commit.

Things I think need adressing:

• The performance fix (Webhook#setable_projects) should be in a separate commit, it is unrelated to the admin page feature.
• secret_hidden? should be named foreign_webhook or something like that since it's also used to decide whether to render the owner or not in the form.
• a new secret an admin tries to set on a webhook they do not own is lost if the form is redisplayed due to validation errors.

Nice to have: pagination seems reasonable at this point, since an admin will see the aggregation of all webhooks systemwide.

Also, for an admin it might be of interest when a webhook was created, so maybe show that in the list as well?

• The performance fix (Webhook#setable_projects) should be in a separate commit, it is unrelated to the admin page feature.

I concur. It should be a separate commit.

Also, with the proposed change, we would now check for the implicit view_project permission (via Project.visible) and the use_webhooks permission. I think, it is sufficient to only check the latter, i.e. to just run Project.allowed_to(user, :use_webhooks).

We should also drop the to_a there. Instead, in the before_validation hook we could just pluck the IDs and compare it to the ids of the hook's proposed projects.

• secret_hidden? should be named foreign_webhook or something like that since it's also used to decide whether to render the owner or not in the form.

Maybe we could reverse the meaning of this method and implement it as an instance method on the actual webhook model as Webhook#own? instead to check if the webhook's user is User.current. That would get rid if the rather awkward controller/view helper method.

However, thinking a bit more about it, I'm not sure if we should use such a helper method at all since it conflates various meanings into a sub-optimally named method. It may be clearer to just check this condition "manually" wherever required.

Finally, I'd like to propose some further changes:

• The check whether to allow setting the secret (now proposed in WebhooksController#webhook_params) should instead be implemented using safe_attributes on the model, similar to most other models in Redmine (as we don't generally use Rails' params filtering framework). Instead of using the plain build or update methods with a "raw" parameters, we would instead call e.g. @webhook.safe_attributes = webhook_params
• Instead of checking which webhooks can be edited in WebhooksController#editable_webhooks, we should instead introduce a new Webhook.editable scope and an accompanying Webhook#editable? method (each receiving an optional user argument which defaults to User.current). The new Webhook.editable scope should then be called in WebhookController#find_webhook. This would follow the approach for all other models.
• I'm not a fan of the ADMIN_CUSTODY_ACTIONS and the accompanying check in check_enabled. Instead, I think we should have separate methods for check_enabled and check_enabled_or_admin wich are then installed as before_filters for the respective actions, e.g.
  

  before_action :check_enabled_or_admin, only: [:edit, :update, :destroy]
  before_action :check_enabled, except: [:edit, :update, :destroy]
  
  def check_enabled
    render_403 unless Webhook.enabled?
  end
  
  def check_enabled_or_admin
    render_403 unless Webhook.enabled? || User.current.admin?
  end
  

Vincent Robert wrote:

The patch also resolves a performance issue by changing one line in Webhook#setable_projects (N+1 queries when filtering lots of projects)

I've created #44386.