fix(epics): use actual group_id for save/delete operations on nested epics #3279

JohnVillalovos · 2025-10-18T17:41:25Z

When an epic belonging to a subgroup is retrieved through a parent
group's epic listing, save() and delete() operations would fail because
they used the parent group's path instead of the epic's actual group_id.

This commit overrides save() and delete() methods in GroupEpic to use
the epic's group_id attribute to construct the correct API path, ensuring
operations work correctly regardless of how the epic was retrieved.

Also add the ability to pass a custom path using _pg_custom_path to
the UpdateMixin.update() and SaveMixin.save() methods. This allowed
the override of the update() method to re-use the SaveMixin.save()
method.

Closes: #3261

Copilot

Pull Request Overview

This PR fixes an issue where save() and delete() operations on epics retrieved through a parent group's epic listing would fail because they used the wrong group path. The fix ensures that operations use the epic's actual group_id attribute to construct the correct API path.

Overrides save() and delete() methods in GroupEpic to use the epic's group_id for API path construction
Adds helper method _epic_path() to compute the correct API path using the epic's real group
Adds comprehensive test coverage for both unit and functional scenarios

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
gitlab/v4/objects/epics.py	Implements the fix by overriding save() and delete() methods to use epic's group_id for path construction
tests/unit/objects/test_epics.py	Adds unit tests to verify save() and delete() operations use the correct group path
tests/functional/api/test_epics.py	Adds functional test demonstrating the fix works for nested epics accessed through parent groups

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

gitlab/v4/objects/epics.py

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

gitlab/mixins.py

gitlab/v4/objects/epics.py

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

gitlab/v4/objects/epics.py

Copilot

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

gitlab/v4/objects/epics.py

codecov · 2025-10-22T00:01:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.76%. Comparing base (c7c139b) to head (0dd6ab1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3279      +/-   ##
==========================================
+ Coverage   95.75%   95.76%   +0.01%     
==========================================
  Files          98       98              
  Lines        6052     6068      +16     
==========================================
+ Hits         5795     5811      +16     
  Misses        257      257

Flag	Coverage Δ
api_func_v4	`83.78% <93.75%> (+0.02%)`	⬆️
cli_func_v4	`78.59% <43.75%> (-0.10%)`	⬇️
unit	`90.21% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
gitlab/mixins.py	`91.55% <100.00%> (+0.04%)`	⬆️
gitlab/v4/objects/epics.py	`91.52% <100.00%> (+2.63%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

JohnVillalovos · 2025-12-06T18:12:16Z

FYI: This was tested and reported as working #3261 (comment)

…epics When an epic belonging to a subgroup is retrieved through a parent group's epic listing, save() and delete() operations would fail because they used the parent group's path instead of the epic's actual group_id. This commit overrides save() and delete() methods in GroupEpic to use the epic's group_id attribute to construct the correct API path, ensuring operations work correctly regardless of how the epic was retrieved. Also add the ability to pass a custom path using `_pg_custom_path` to the `UpdateMixin.update()` and `SaveMixin.save()` methods. This allowed the override of the `update()` method to re-use the `SaveMixin.save()` method. Closes: #3261

nejch

Thanks @JohnVillalovos, looks good and that's a tricky one. Not sure about lazily fetched epics - just have a few questions there.

nejch · 2025-12-24T22:54:18Z

gitlab/v4/objects/epics.py


+ 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:


Can a group_id actually be none for an epic, since it's always a group resource? Or can we go with a simple try/except here?

One other thought - will this work if we get an epic with lazy=True from an arbitrary group?

nejch · 2025-12-24T22:57:07Z

gitlab/v4/objects/epics.py

+ path = self._epic_path()
+
+ # Call SaveMixin.save() method
+ return super().save(_pg_custom_path=path, **kwargs)


Not sure if we should have some more generic _get_custom_path or _resolve_path helpers that would automatically get picked up by the mixins, as we already do this twice for delete/update. That way we don't need an override here (or maybe also in some other places), but I'm not sure if feasible or worth it.

nejch · 2025-12-24T22:58:11Z

gitlab/mixins.py

 path = self.path
 else:
 path = f"{self.path}/{utils.EncodedId(id)}"
+ if "_pg_custom_path" in kwargs:


nit: pg_ always makes me think of postgres here, but maybe I've spent too much time in gitlab.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?

Suggested change

if "_pg_custom_path" in kwargs:

if "_custom_path" in kwargs:

nejch · 2025-12-24T23:02:49Z

tests/functional/api/test_epics.py

+        }
+    )
+
+ nested_epic = subgroup.epics.create(


Could we have this one as a fixture that yields the epic, to avoid the manual cleanup below?

Or I'm just confused by the try/finally block, maybe it was meant to test deletion even if previous things failed.

Copilot AI review requested due to automatic review settings October 18, 2025 17:41

JohnVillalovos marked this pull request as draft October 18, 2025 17:41

Copilot AI reviewed Oct 18, 2025

View reviewed changes

gitlab/v4/objects/epics.py Outdated Show resolved Hide resolved

gitlab/v4/objects/epics.py Outdated Show resolved Hide resolved

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from 0f7065d to 3e75d60 Compare October 21, 2025 22:59

JohnVillalovos requested a review from Copilot October 21, 2025 23:02

Copilot AI reviewed Oct 21, 2025

View reviewed changes

gitlab/mixins.py Outdated Show resolved Hide resolved

gitlab/v4/objects/epics.py Outdated Show resolved Hide resolved

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from 3e75d60 to 3993fe7 Compare October 21, 2025 23:05

JohnVillalovos requested a review from Copilot October 21, 2025 23:06

Copilot AI reviewed Oct 21, 2025

View reviewed changes

gitlab/v4/objects/epics.py Outdated Show resolved Hide resolved

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch 2 times, most recently from 3e855f3 to 27bd80c Compare October 21, 2025 23:33

JohnVillalovos requested a review from Copilot October 21, 2025 23:35

Copilot AI reviewed Oct 21, 2025

View reviewed changes

gitlab/v4/objects/epics.py Show resolved Hide resolved

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from 27bd80c to dcc9847 Compare October 21, 2025 23:42

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from dcc9847 to f420862 Compare October 22, 2025 00:25

JohnVillalovos requested a review from Copilot October 22, 2025 00:26

Copilot AI reviewed Oct 22, 2025

View reviewed changes

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from f420862 to a9638ee Compare October 22, 2025 00:42

JohnVillalovos mentioned this pull request Oct 22, 2025

GroupEpic uses "iid" as ID attribute but should use "id"? #3261

Open

JohnVillalovos marked this pull request as ready for review November 10, 2025 16:02

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from a9638ee to fb4f902 Compare November 10, 2025 16:02

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from fb4f902 to 8397bd0 Compare December 6, 2025 18:11

JohnVillalovos requested a review from nejch December 6, 2025 18:12

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from 8397bd0 to 45a8599 Compare December 11, 2025 00:56

JohnVillalovos force-pushed the jlvillal/3261_epic_group branch from 45a8599 to 0dd6ab1 Compare December 24, 2025 21:42

nejch reviewed Dec 24, 2025

View reviewed changes

fix(epics): use actual group_id for save/delete operations on nested epics #3279

Are you sure you want to change the base?

fix(epics): use actual group_id for save/delete operations on nested epics #3279

Conversation

JohnVillalovos commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

codecov bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

JohnVillalovos commented Dec 6, 2025

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

nejch Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

nejch Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

nejch Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

nejch Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JohnVillalovos commented Oct 18, 2025 •

edited

Loading

codecov bot commented Oct 22, 2025 •

edited

Loading