Add more webhooks support #82
Conversation
|
|
||
| private | ||
|
|
||
| def camelize(term) |
There was a problem hiding this comment.
I'm wondering if it would make sense to copy the Rails' implementation as that's what I'd expect most people using the gem are familiar with. Otherwise there might be subtle differences.
See https://apidock.com/rails/v6.1.3.1/String/camelize. Rails 7's is harder to read in my opinion: https://github.com/rails/rails/blob/de53ba56cab69fb9707785a397a59ac4aaee9d6f/activesupport/lib/active_support/inflector/methods.rb#L96.
Maybe it would also be nice to extract the function to a module as camelize is a concept that makes sense for all parts of the app.
What do you think @eikes?
There was a problem hiding this comment.
It is in fact copied from rails 7 here: https://github.com/rails/rails/blob/de53ba56cab69fb9707785a397a59ac4aaee9d6f/activesupport/lib/active_support/inflector/methods.rb#L69
But I had to remove everything related to inflections. Finally I had to replace the $1 and $2, because rubocop didn't like that.
In general though, I'd prefer to use ActiveSupport, but I was told not to. What are your thoughts on using integrating ActiveSupport? It would let us remove some reimplementations like this and everyone uses it anyways...
There was a problem hiding this comment.
Huh. I looked at that method as well and didn't recognise it. Odd. I'd personally be Ok with ActiveSupport, but was also told not to use it and I'd stick with that. I think the next best option would be to have a module for "the stuff copied from Rails" with some comments and links to the original source.
There was a problem hiding this comment.
OK, let's create a Ioki::Support module after merging this!
Co-authored-by: Andreas Hellwig <a5308y@gmail.com>
Webhooks require a validation of the
x-signatureHTTP header which is implemented here.Additionally a helper class to convert the incoming webhook event into a Ioki::Webhooks::Model model is provided.