Skip to content
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.

Sign up for GitHub

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

[IMP] make _set_lines_issue working on empty issue #190

Merged
merged 2 commits into from
Jul 3, 2022
Merged

[IMP] make _set_lines_issue working on empty issue #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e707c6
[IMP] make _set_lines_issue working on empty issue
legalsylvain Jul 1, 2022
4ba3d91
[REF] refactor to improve _set_lines_issue coverage
legalsylvain Jul 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions newsfragments/190.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make the command ``/ocabot migration`` working when migration issue doesn't contain migration module lines.
18 changes: 12 additions & 6 deletions src/oca_github_bot/tasks/migration_issue_bot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ def _find_issue(gh_repo, milestone, target_branch):
return issue


def _set_lines_issue(gh_pr, issue, module):
def _set_lines_issue(gh_pr_user_login, gh_pr_number, issue_body, module):
lines = []
added = False
module_list = False
new_line = f"- [ ] {module} - By @{gh_pr.user.login} - #{gh_pr.number}"
for line in issue.body.split("\n"):
new_line = f"- [ ] {module} - By @{gh_pr_user_login} - #{gh_pr_number}"
for line in issue_body.split("\n"):
if added: # Bypass the checks for faster completion
lines.append(line)
continue
Expand All @@ -58,7 +58,11 @@ def _set_lines_issue(gh_pr, issue, module):
lines.append(new_line)
added = True
lines.append(line)
return lines

# make the addition working on an empty migration issue
if not added:
lines.append(new_line)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typical migration issue ends with comments after the check list of modules:

image

So this would add a line after the comment which may not look very nice.

I wish we had better tests for _set_lines_issue.

Copy link
Collaborator Author

@legalsylvain legalsylvain Jul 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typical migration issue ends with comments after the check list of modules:

that's not the point of the PR.

The line will be added only if the issue doesn't contain module list.
In your screenshot, my patch has no effect, because previous code will add the line in the correct place, and so added will be true.

That is just a trivial patch to initialize migration issue, if there is no module list.

I wish we had better tests for _set_lines_issue.

Well. Yes it's possible. The simplier is to change the function _set_lines_issue(gh_pr, issue, module) by _set_lines_issue(gh_pr_number, gh_pr_login, body_issue, module)``
So simple and precise test can be written, covering all the cases. (without pointing on existing PR as this is the case for the time being https://github.com/OCA/oca-github-bot/blob/master/tests/test_migration_issue_bot.py#L37).
with the current design it is not possible to test the result of the function, because the result is not predictive.

but I have 3 pending PR around _set_lines_issue so I can do this refactor, but if it's possible, after having merged The current PR.

return "\n".join(lines)


@task()
Expand Down Expand Up @@ -109,8 +113,10 @@ def migration_issue_start(org, repo, pr, username, module=None, dry_run=False):
)
return
# Change issue to add the PR in the module list
lines = _set_lines_issue(gh_pr, issue, module)
issue.edit(body="\n".join(lines))
new_body = _set_lines_issue(
gh_pr.user.login, gh_pr.number, issue.body, module
)
issue.edit(body=new_body)
except Exception as e:
github.gh_call(
gh_pr.create_comment,
Expand Down
37 changes: 31 additions & 6 deletions tests/test_migration_issue_bot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,34 @@ def test_find_issue(gh):

@pytest.mark.vcr()
def test_set_lines_issue(gh):
repo = _get_repository(gh, "OCA", "contract")
milestone = _create_or_find_branch_milestone(repo, "14.0")
issue = _find_issue(repo, milestone, "14.0")
gh_pr = gh.pull_request("OCA", "contract", 705)
lines = _set_lines_issue(gh_pr, issue, "contract")
assert len(lines) > 0
module = "mis_builder"
gh_pr_user_login = "sbidoul"
gh_pr_number = 11

body_transformation = [
(
"Issue with list but not the module\n"
"- [ ] a_module_1 - By @legalsylvain - #1\n"
"- [ ] z_module_1 - By @pedrobaeza - #2",
f"Issue with list but not the module\n"
f"- [ ] a_module_1 - By @legalsylvain - #1\n"
f"- [ ] {module} - By @{gh_pr_user_login} - #{gh_pr_number}\n"
f"- [ ] z_module_1 - By @pedrobaeza - #2",
),
(
f"Issue with list containing the module\n"
f"- [x] {module} - By @legalsylvain - #1\n"
f"- [ ] z_module_1 - By @pedrobaeza - #2",
f"Issue with list containing the module\n"
f"- [x] {module} - By @{gh_pr_user_login} - #{gh_pr_number}\n"
f"- [ ] z_module_1 - By @pedrobaeza - #2",
),
(
"Issue with no list",
f"Issue with no list\n"
f"- [ ] {module} - By @{gh_pr_user_login} - #{gh_pr_number}",
),
]
for (old_body, new_body_expected) in body_transformation:
new_body = _set_lines_issue(gh_pr_user_login, gh_pr_number, old_body, module)
assert new_body == new_body_expected