-
Notifications
You must be signed in to change notification settings - Fork 675
fix(epics): use actual group_id for save/delete operations on nested epics #3279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| from typing import Any, TYPE_CHECKING | ||
|
|
||
| import gitlab.utils | ||
| from gitlab import exceptions as exc | ||
| from gitlab import types | ||
| from gitlab.base import RESTObject | ||
|
|
@@ -29,6 +30,63 @@ class GroupEpic(ObjectDeleteMixin, SaveMixin, RESTObject): | |
| resourcelabelevents: GroupEpicResourceLabelEventManager | ||
| notes: GroupEpicNoteManager | ||
|
|
||
| def _epic_path(self) -> str: | ||
| """Return the API path for this epic using its real group.""" | ||
| if not hasattr(self, "group_id") or self.group_id is None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can a One other thought - will this work if we get an epic with |
||
| raise AttributeError( | ||
| "Cannot compute epic path: attribute 'group_id' is missing." | ||
| ) | ||
| encoded_group_id = gitlab.utils.EncodedId(self.group_id) | ||
| return f"/groups/{encoded_group_id}/epics/{self.encoded_id}" | ||
|
|
||
| @exc.on_http_error(exc.GitlabUpdateError) | ||
| def save(self, **kwargs: Any) -> dict[str, Any] | None: | ||
| """Save the changes made to the object to the server. | ||
|
|
||
| The object is updated to match what the server returns. | ||
|
|
||
| This method uses the epic's group_id attribute to construct the correct | ||
| API path. This is important when the epic was retrieved from a parent | ||
| group but actually belongs to a sub-group. | ||
|
|
||
| Args: | ||
| **kwargs: Extra options to send to the server (e.g. sudo) | ||
|
|
||
| Returns: | ||
| The new object data (*not* a RESTObject) | ||
|
|
||
| Raises: | ||
| GitlabAuthenticationError: If authentication is not correct | ||
| GitlabUpdateError: If the server cannot perform the request | ||
| """ | ||
| # Use the epic's actual group_id to construct the correct path. | ||
| path = self._epic_path() | ||
|
|
||
| # Call SaveMixin.save() method | ||
| return super().save(_pg_custom_path=path, **kwargs) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if we should have some more generic |
||
|
|
||
| @exc.on_http_error(exc.GitlabDeleteError) | ||
| def delete(self, **kwargs: Any) -> None: | ||
| """Delete the object from the server. | ||
|
|
||
| This method uses the epic's group_id attribute to construct the correct | ||
| API path. This is important when the epic was retrieved from a parent | ||
| group but actually belongs to a sub-group. | ||
|
|
||
| Args: | ||
| **kwargs: Extra options to send to the server (e.g. sudo) | ||
|
|
||
| Raises: | ||
| GitlabAuthenticationError: If authentication is not correct | ||
| GitlabDeleteError: If the server cannot perform the request | ||
| """ | ||
| if TYPE_CHECKING: | ||
| assert self.encoded_id is not None | ||
|
|
||
| # Use the epic's actual group_id to construct the correct path. | ||
| path = self._epic_path() | ||
| self.manager.gitlab.http_delete(path, **kwargs) | ||
|
|
||
|
|
||
| class GroupEpicManager(CRUDMixin[GroupEpic]): | ||
| _path = "/groups/{group_id}/epics" | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,10 @@ | ||
| import uuid | ||
|
|
||
| import pytest | ||
|
|
||
| import gitlab.exceptions | ||
| from tests.functional import helpers | ||
|
|
||
| pytestmark = pytest.mark.gitlab_premium | ||
|
|
||
|
|
||
|
|
@@ -32,3 +37,42 @@ def test_epic_notes(epic): | |
| epic.notes.create({"body": "Test note"}) | ||
| new_notes = epic.notes.list(get_all=True) | ||
| assert len(new_notes) == (len(notes) + 1), f"{new_notes} {notes}" | ||
|
|
||
|
|
||
| def test_epic_save_from_parent_group_updates_subgroup_epic(gl, group): | ||
| subgroup_id = uuid.uuid4().hex | ||
| subgroup = gl.groups.create( | ||
| { | ||
| "name": f"subgroup-{subgroup_id}", | ||
| "path": f"sg-{subgroup_id}", | ||
| "parent_id": group.id, | ||
| } | ||
| ) | ||
|
|
||
| nested_epic = subgroup.epics.create( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we have this one as a fixture that Or I'm just confused by the try/finally block, maybe it was meant to test deletion even if previous things failed. |
||
| {"title": f"Nested epic {subgroup_id}", "description": "Nested epic"} | ||
| ) | ||
|
|
||
| try: | ||
| fetched_epics = group.epics.list(search=nested_epic.title) | ||
| assert fetched_epics, "Expected to discover nested epic via parent group list" | ||
| subgroup.epics.get(nested_epic.iid) | ||
|
|
||
| fetched_epic = next( | ||
| (epic for epic in fetched_epics if epic.id == nested_epic.id), None | ||
| ) | ||
| assert ( | ||
| fetched_epic is not None | ||
| ), "Parent group listing did not include nested epic" | ||
|
|
||
| new_label = f"nested-{subgroup_id}" | ||
| fetched_epic.labels = [new_label] | ||
| fetched_epic.save() | ||
|
|
||
| refreshed_epic = subgroup.epics.get(nested_epic.iid) | ||
| assert new_label in refreshed_epic.labels | ||
| finally: | ||
| helpers.safe_delete(nested_epic) | ||
| with pytest.raises(gitlab.exceptions.GitlabGetError): | ||
| subgroup.epics.get(nested_epic.iid) | ||
| helpers.safe_delete(subgroup) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import pytest | ||
| import responses | ||
|
|
||
| from gitlab.v4.objects.epics import GroupEpic | ||
|
|
||
|
|
||
| def _build_epic(manager, iid=3, group_id=2, title="Epic"): | ||
| data = {"iid": iid, "group_id": group_id, "title": title} | ||
| return GroupEpic(manager, data) | ||
|
|
||
|
|
||
| def test_group_epic_save_uses_actual_group_path(group): | ||
| epic_manager = group.epics | ||
| epic = _build_epic(epic_manager, title="Original") | ||
| epic.title = "Updated" | ||
|
|
||
| with responses.RequestsMock() as rsps: | ||
| rsps.add( | ||
| method=responses.PUT, | ||
| url="http://localhost/api/v4/groups/2/epics/3", | ||
| json={"iid": 3, "group_id": 2, "title": "Updated"}, | ||
| content_type="application/json", | ||
| status=200, | ||
| match=[responses.matchers.json_params_matcher({"title": "Updated"})], | ||
| ) | ||
|
|
||
| epic.save() | ||
|
|
||
| assert epic.title == "Updated" | ||
|
|
||
|
|
||
| def test_group_epic_delete_uses_actual_group_path(group): | ||
| epic_manager = group.epics | ||
| epic = _build_epic(epic_manager) | ||
|
|
||
| with responses.RequestsMock() as rsps: | ||
| rsps.add( | ||
| method=responses.DELETE, | ||
| url="http://localhost/api/v4/groups/2/epics/3", | ||
| status=204, | ||
| ) | ||
|
|
||
| epic.delete() | ||
|
|
||
| assert len(epic._updated_attrs) == 0 | ||
|
|
||
|
|
||
| def test_group_epic_path_requires_group_id(fake_manager): | ||
| epic = GroupEpic(manager=fake_manager, attrs={"iid": 5}) | ||
|
|
||
| with pytest.raises(AttributeError): | ||
| epic._epic_path() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
pg_always makes me think of postgres here, but maybe I've spent too much time ingitlab.rb. Also depending on the answer in the other questions, would it make sense to have this as a keyword argument instead of popping it here?