Moved the webhooks and services gear options to a single one called integrations
What does this MR do?
Are there points in the code the reviewer needs to double check?
Why was this MR needed?
Screenshots (if relevant)
Both webhooks and services together
Gear button
Does this MR meet the acceptance criteria?
-
Changelog entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Merge request reports
Activity
\cc @tauriedavis
added 1 commit
- 9fd79717 - Fixed spinach feature tests for the services and hooks controllers
assigned to @alfredo1
@alfredo1 Thanks for reviewing this by the way!
Awesome this looks great @jivanvl !
@jivanvl please rebase with master and fix conflicts
assigned to @jivanvl
added 232 commits
-
0d0bf5d2...9cdfcbb5 - 229 commits from branch
master
- d78b9a9f - Moved the webhooks and services gear options to a single one called integrations
- e488f26f - Fixed spinach feature tests for the services and hooks controllers
- a4dfca53 - Fixed more tests
Toggle commit list-
0d0bf5d2...9cdfcbb5 - 229 commits from branch
@alfredo1 Rebased and fixed conflicts
assigned to @alfredo1
@jivanvl is giving me an error when I try to add a new webhook when I submit the form directly.
changed milestone to %8.16
@jivanvl Frontend looks good. Just fix the issue I mentioned and please get the ruby part to be reviewed by a backend endboss.
assigned to @jivanvl
I have this problem right now where all of the tests pass but I don't think we test for this. For example in master if you add a hook with an invalid URL this message gets shown with the
form_errors
helperAnd now using this particular helper needs a
hook
model to actually show the error, which is difficult to get due to the fact that now we have two controllers, one for the setting page itself which isIntegrationsController
and another one that actually has the create logic on it which is theHooksController
, there's a couple a solutions that come in mind and that Ruben helped me out figure.- We move the
create
logic from theHooksController
to theIntegrationsController
this way we can reuse theform_errors
helper no problem and keep the rest of theHooksController
intact. - I change the Webhooks form to use AJAX to make a request to the
HooksController
create
action so it responds with a JSON string and process it on javascript. - We use presenters for this particular MR but this would require an available backend developer to help me out plus I think it might require the MR !8480 (merged) to be merged.
- Use
flash
to show the errors, but this would make the consistency across form errors be out of place.
What do you think @grzesiek? Are there any perhaps other options to tackle this problem?
- We move the
assigned to @grzesiek
@jivanvl The first thing that comes to my mind is using presenters along with objects used to operate on settings. I mean something like having a class for each settings group
class Gitlab::Settings::Hooks
which would handle all operations, including saving settings, exposing validation errors, includingPresentable
(after !8480 (merged) is merged) and exposing presenter throughpresent
method.You are right, and this would indeed require using presenters and doing a lot of additional backend work. If we want to avoid it, in my opinion using
flash
alerts might be the most boring solution, but it also increases a technical debt we will leave behind.- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
@jivanvl I left a couple of notes. Great work!
assigned to @jivanvl
- Resolved by Jose Ivan Vargas
@grzesiek Improved the MR based on your observations, as for the
flash
solution I guess that would be a way of fixing the issue but I can't say for sure if we want more technical debt. Do you think that we should reach someone else for their input?Also thanks!
Edited by Jose Ivan Vargasassigned to @grzesiek
@jivanvl I'm a little primed by a strong belief that we should not leave significant technical debt behind. So if you ask me, I would probably consider improving it now, but we have already at least one similar MR merged, so this wouldn't be consistent. The point here is that we should pay off this debt as soon as possible. I think that using
flash
messages is good as long as we are going to resolve https://gitlab.com/gitlab-org/gitlab-ce/issues/26255 soon. What do you think @rymai?Edited by Grzegorz Bizon@jivanvl Looks good to me!
assigned to @jivanvl
@jivanvl: Good for 8.16?
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
@victorwu I think so, I just need to add the finishing touches and have it reviewed once more!
Thanks @jivanvl !
added 1 commit
- 6b684449 - Removed the index action from both the projects hook and services controllers
added 1 commit
- 6b684449 - Removed the index action from both the projects hook and services controllers
added 851 commits
-
6b684449...61b6643e - 845 commits from branch
master
- 9f0d7945 - Moved the webhooks and services gear options to a single one called integrations
- 1ee4f986 - Fixed spinach feature tests for the services and hooks controllers
- bc7c6c68 - Fixed more tests
- 373411d1 - Corrected code style and titles
- 0af99433 - Removed the index action from both the projects hook and services controllers
- 7b3ea49a - Fixed tests and a rubocop linter
Toggle commit list-
6b684449...61b6643e - 845 commits from branch
@rymai Improved the MR based on your comments, can you take a look once more when you get a chance?
Thanks
@jivanvl LGTM but please open a MR against EE as well (see https://gitlab.com/gitlab-org/gitlab-ce/builds/8916353)!