<feed xmlns='http://www.w3.org/2005/Atom'>
<title>delta/gitlab/gitlab-ce.git/app/services/protected_branches, branch commit-description-border</title>
<subtitle>gitlab.com: gitlab-org/gitlab-ce.git
</subtitle>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/'/>
<entry>
<title>Implement third round of review comments from @DouweM.</title>
<updated>2016-10-24T06:03:38+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-10-24T06:02:09+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=db0182e261936c3e800b546d307a3d3834ff9927'/>
<id>db0182e261936c3e800b546d307a3d3834ff9927</id>
<content type='text'>
Extract/mutate `params` in the `execute` method of the API services,
rather than in `initialize`.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Extract/mutate `params` in the `execute` method of the API services,
rather than in `initialize`.
</pre>
</div>
</content>
</entry>
<entry>
<title>Implement second round of review comments from @DouweM.</title>
<updated>2016-10-24T06:03:38+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-09-30T07:44:46+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=1051087ac4efc3dbf45bd075e36af647d2b66d62'/>
<id>1051087ac4efc3dbf45bd075e36af647d2b66d62</id>
<content type='text'>
- Pass `developers_and_merge` and `developers_can_push` in `params`
  instead of using keyword arguments.

- Refactor a slightly complex boolean check to a simple `nil?` check.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
- Pass `developers_and_merge` and `developers_can_push` in `params`
  instead of using keyword arguments.

- Refactor a slightly complex boolean check to a simple `nil?` check.
</pre>
</div>
</content>
</entry>
<entry>
<title>Implement review comments from @DouweM.</title>
<updated>2016-10-24T06:03:38+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-09-22T15:08:05+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=b803bc7bb8ad481790d01848902e80602e77da67'/>
<id>b803bc7bb8ad481790d01848902e80602e77da67</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>Implement review comments from @dbalexandre.</title>
<updated>2016-10-24T06:03:38+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-09-14T02:34:29+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=cef10ef7d7a20a78d377f711867e361bb51fbaf2'/>
<id>cef10ef7d7a20a78d377f711867e361bb51fbaf2</id>
<content type='text'>
1. Don't have any EE-only code in CE. Ok to have CE-only code in EE.

2. Use `case` instead of `if/elsif`
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
1. Don't have any EE-only code in CE. Ok to have CE-only code in EE.

2. Use `case` instead of `if/elsif`
</pre>
</div>
</content>
</entry>
<entry>
<title>Fix branch protection API.</title>
<updated>2016-10-24T06:03:38+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-09-06T05:05:34+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=f79f3a1dd621362b0894eff0a54f220f8415a2e0'/>
<id>f79f3a1dd621362b0894eff0a54f220f8415a2e0</id>
<content type='text'>
1. Previously, we were not removing existing access levels before
   creating new ones. This is not a problem for EE, but _is_ for CE,
   since we restrict the number of access levels in CE to 1.

2. The correct approach is:

    CE -&gt; delete all access levels before updating a protected branch
    EE -&gt; delete developer access levels if "developers_can_{merge,push}" is switched off

3. The dispatch is performed by checking if a "length: 1" validation is
   present on the access levels or not.

4. Another source of problems was that we didn't put multiple queries in
   a transaction. If the `destroy_all` passes, but the `update` fails,
   we should have a rollback.

5. Modifying the API to provide users direct access to CRUD access
   levels will make things a lot simpler.

6. Create `create/update` services separately for this API, which
   perform the necessary data translation, before calling the regular
   `create/update` services. The translation code was getting too large
   for the API endpoint itself, so this move makes sense.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
1. Previously, we were not removing existing access levels before
   creating new ones. This is not a problem for EE, but _is_ for CE,
   since we restrict the number of access levels in CE to 1.

2. The correct approach is:

    CE -&gt; delete all access levels before updating a protected branch
    EE -&gt; delete developer access levels if "developers_can_{merge,push}" is switched off

3. The dispatch is performed by checking if a "length: 1" validation is
   present on the access levels or not.

4. Another source of problems was that we didn't put multiple queries in
   a transaction. If the `destroy_all` passes, but the `update` fails,
   we should have a rollback.

5. Modifying the API to provide users direct access to CRUD access
   levels will make things a lot simpler.

6. Create `create/update` services separately for this API, which
   perform the necessary data translation, before calling the regular
   `create/update` services. The translation code was getting too large
   for the API endpoint itself, so this move makes sense.
</pre>
</div>
</content>
</entry>
<entry>
<title>Backport changes from gitlab-org/gitlab-ee!581 to CE.</title>
<updated>2016-08-16T05:35:14+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-08-16T05:09:13+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=e805a6470031d942f7de604fdf7acfc7cf4f0b1a'/>
<id>e805a6470031d942f7de604fdf7acfc7cf4f0b1a</id>
<content type='text'>
!581 has a lot of changes that would cause merge conflicts if not
properly backported to CE. This commit/MR serves as a better
foundation for gitlab-org/gitlab-ee!581.

= Changes =

1. Move from `has_one {merge,push}_access_level` to `has_many`, with the
   `length` of the association limited to `1`. This is _effectively_ a
   `has_one` association, but should cause less conflicts with EE, which
   is set to `has_many`. This has a number of related changes in the
   views, specs, and factories.

2. Make `gon` variable loading more consistent (with EE!581) in the
   `ProtectedBranchesController`. Also use `::` to prefix the
   `ProtectedBranches` services, because this is required in EE.

3. Extract a `ProtectedBranchAccess` concern from the two access level
   models. This concern only has a single `humanize` method here, but
   will have more methods in EE.

4. Add `form_errors` to the protected branches creation form. This is
   not strictly required for EE compatibility, but was an oversight
   nonetheless.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
!581 has a lot of changes that would cause merge conflicts if not
properly backported to CE. This commit/MR serves as a better
foundation for gitlab-org/gitlab-ee!581.

= Changes =

1. Move from `has_one {merge,push}_access_level` to `has_many`, with the
   `length` of the association limited to `1`. This is _effectively_ a
   `has_one` association, but should cause less conflicts with EE, which
   is set to `has_many`. This has a number of related changes in the
   views, specs, and factories.

2. Make `gon` variable loading more consistent (with EE!581) in the
   `ProtectedBranchesController`. Also use `::` to prefix the
   `ProtectedBranches` services, because this is required in EE.

3. Extract a `ProtectedBranchAccess` concern from the two access level
   models. This concern only has a single `humanize` method here, but
   will have more methods in EE.

4. Add `form_errors` to the protected branches creation form. This is
   not strictly required for EE compatibility, but was an oversight
   nonetheless.
</pre>
</div>
</content>
</entry>
<entry>
<title>Implement final review comments from @rymai.</title>
<updated>2016-07-29T09:50:39+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-07-29T07:01:15+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=cebcc417eda08711ad17a433d6d9b4f49830c04c'/>
<id>cebcc417eda08711ad17a433d6d9b4f49830c04c</id>
<content type='text'>
1. Instantiate `ProtectedBranchesAccessSelect` from `dispatcher`

2. Use `can?(user, ...)` instead of `user.can?(...)`

3. Add `DOWNTIME` notes to all migrations added in !5081.

4. Add an explicit `down` method for migrations removing the
   `developers_can_push` and `developers_can_merge` columns, ensuring that
   the columns created (on rollback) have the appropriate defaults.

5. Remove duplicate CHANGELOG entries.

6. Blank lines after guard clauses.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
1. Instantiate `ProtectedBranchesAccessSelect` from `dispatcher`

2. Use `can?(user, ...)` instead of `user.can?(...)`

3. Add `DOWNTIME` notes to all migrations added in !5081.

4. Add an explicit `down` method for migrations removing the
   `developers_can_push` and `developers_can_merge` columns, ensuring that
   the columns created (on rollback) have the appropriate defaults.

5. Remove duplicate CHANGELOG entries.

6. Blank lines after guard clauses.
</pre>
</div>
</content>
</entry>
<entry>
<title>Use `Gitlab::Access` to protected branch access levels.</title>
<updated>2016-07-29T09:50:39+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-07-29T06:13:07+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=0a8aeb46dc187cc309ddbe23d8624f5d24b6218c'/>
<id>0a8aeb46dc187cc309ddbe23d8624f5d24b6218c</id>
<content type='text'>
1. It makes sense to reuse these constants since we had them duplicated
   in the previous enum implementation. This also simplifies our
   `check_access` implementation, because we can use
   `project.team.max_member_access` directly.

2. Use `accepts_nested_attributes_for` to create push/merge access
   levels. This was a bit fiddly to set up, but this simplifies our code
   by quite a large amount. We can even get rid of
   `ProtectedBranches::BaseService`.

3. Move API handling back into the API (previously in
   `ProtectedBranches::BaseService#translate_api_params`.

4. The protected branch services now return a `ProtectedBranch` rather
   than `true/false`.

5. Run `load_protected_branches` on-demand in the `create` action, to
   prevent it being called unneccessarily.

6. "Masters" is pre-selected as the default option for "Allowed to Push"
   and "Allowed to Merge".

7. These changes were based on a review from @rymai in !5081.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
1. It makes sense to reuse these constants since we had them duplicated
   in the previous enum implementation. This also simplifies our
   `check_access` implementation, because we can use
   `project.team.max_member_access` directly.

2. Use `accepts_nested_attributes_for` to create push/merge access
   levels. This was a bit fiddly to set up, but this simplifies our code
   by quite a large amount. We can even get rid of
   `ProtectedBranches::BaseService`.

3. Move API handling back into the API (previously in
   `ProtectedBranches::BaseService#translate_api_params`.

4. The protected branch services now return a `ProtectedBranch` rather
   than `true/false`.

5. Run `load_protected_branches` on-demand in the `create` action, to
   prevent it being called unneccessarily.

6. "Masters" is pre-selected as the default option for "Allowed to Push"
   and "Allowed to Merge".

7. These changes were based on a review from @rymai in !5081.
</pre>
</div>
</content>
</entry>
<entry>
<title>Authorize user before creating/updating a protected branch.</title>
<updated>2016-07-29T09:50:39+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-07-27T04:44:38+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=6d841eaadcbccfa4527bd892bf86fc8dbba19455'/>
<id>6d841eaadcbccfa4527bd892bf86fc8dbba19455</id>
<content type='text'>
1. This is a third line of defence (first in the view, second in the
   controller).

2. Duplicate the `API::Helpers.to_boolean` method in `BaseService`. The
   other alternative is to `include API::Helpers`, but this brings with it
   a number of other methods that might cause conflicts.

3. Return a 403 if authorization fails.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
1. This is a third line of defence (first in the view, second in the
   controller).

2. Duplicate the `API::Helpers.to_boolean` method in `BaseService`. The
   other alternative is to `include API::Helpers`, but this brings with it
   a number of other methods that might cause conflicts.

3. Return a 403 if authorization fails.
</pre>
</div>
</content>
</entry>
<entry>
<title>Have the `branches` API work with the new protected branches data model.</title>
<updated>2016-07-29T09:50:39+00:00</updated>
<author>
<name>Timothy Andrew</name>
<email>mail@timothyandrew.net</email>
</author>
<published>2016-07-25T14:44:53+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/gitlab/gitlab-ce.git/commit/?id=01d190a84ad9b8e4a40cbdec8a55946bac38ab76'/>
<id>01d190a84ad9b8e4a40cbdec8a55946bac38ab76</id>
<content type='text'>
1. The new data model moves from `developers_can_{push,merge}` to
   `allowed_to_{push,merge}`.

2. The API interface has not been changed. It still accepts
   `developers_can_push` and `developers_can_merge` as options. These
   attributes are inferred from the new data model.

3. Modify the protected branch create/update services to translate from
   the API interface to our current data model.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
1. The new data model moves from `developers_can_{push,merge}` to
   `allowed_to_{push,merge}`.

2. The API interface has not been changed. It still accepts
   `developers_can_push` and `developers_can_merge` as options. These
   attributes are inferred from the new data model.

3. Modify the protected branch create/update services to translate from
   the API interface to our current data model.
</pre>
</div>
</content>
</entry>
</feed>
