[IMP] make _set_lines_issue working on empty issue

[legalsylvain]

Hi.

when calling /ocabot migration, if the issue doesn't contains lines, the command fail silently. (that is due to the github api I guess. If we "edit" an github issue, and write the same text as before, the write is ignored).

with that patch, the line is added at the end of the migration issue body.

Context

• can be used on custom modules, if we don't want to initialize the repository.
• usefull also in the Openupgrade project (See : [RFC] Content of Migration Issues and use of ocabot migration command OpenUpgrade#3288 (comment)
  with the decision to not populate migration issue).

CC : @pedrobaeza, @sbidoul

thanks for your review.

[HaraldPanten]

Hi @legalsylvain thank you for this work.

If I understood well, if we call the ocabot migration command in a migration PR that has not been added in the issue before, a new line will be added with the name of the module, right?

My suggestion is... Why not adding it by alphabetical order? Is it possible?

THX!

[legalsylvain]

Hi @HaraldPanten

hi. thanks for your review.
i'm not sure to understand correctly your remark.

• the current PR is about the possibility to add a line if no lines are present in the migration issue. So, as it is the first element, there is nothing to order alphabetically.
• regarding the insertion of a new line, if lines are present, it already works. see the historic of this PR : Migration to version 15.0 OpenUpgrade#3287

[sbidoul]

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

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.

[legalsylvain]

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.

[legalsylvain]

@sbidoul. I refactored the function to make explicit tests. (coverage: +0,5%)

Given a PR '11' made by 'sbidoul' user regarding 'mis_builder' module.

Issue with list but not the module
- [ ] a_module_1 - By @legalsylvain - #1
- [ ] z_module_1 - By @pedrobaeza - #2

become

Issue with list but not the module
- [ ] a_module_1 - By @legalsylvain - #1
- [ ] mis_builder - By @sbidoul - #11
- [ ] z_module_1 - By @pedrobaeza - #2

Issue with list containing the module
- [x] mis_builder - By @legalsylvain - #1
- [ ] z_module_1 - By @pedrobaeza - #2

become

Issue with list containing the module
- [x] mis_builder - By @sbidoul - #11
- [ ] z_module_1 - By @pedrobaeza - #2

Issue with no list

become

Issue with no list
- [ ] mis_builder - By @sbidoul - #11

Note : Use case 1 & 2 are the current behaviour.
The patch of this PR (first commit) allow the Use case 3.
(without that patch, Issue with no list become Issue with no list.

Let me know if it's not clear.

[sbidoul]

Thanks a lot for the refactoring and test, Sylvain. It's much clearer now :)
Can you also add the news fragment ?

[legalsylvain]

Hi @sbidoul. I added a 190.bugfix fragment.

Question : there is pending 183.bugfix file that has not disappeared even if the PR #183 is merged. Is it normal ?

[sbidoul]

Question : there is pending 183.bugfix file that has not disappeared even if the PR #183 is merged. Is it normal ?

Yes, they disappear when we run towncrier at release time.

[legalsylvain]

Yes, they disappear when we run towncrier at release time.

Thanks ! I thought it was during merge process.

[HaraldPanten]

Hi @HaraldPanten

hi. thanks for your review. i'm not sure to understand correctly your remark.

• the current PR is about the possibility to add a line if no lines are present in the migration issue. So, as it is the first element, there is nothing to order alphabetically.
• regarding the insertion of a new line, if lines are present, it already works. see the historic of this PR : Migration to version 15.0 OpenUpgrade#3287

Hi @legalsylvain Sorry, probably I misunderstood the functionality.

Thanks for the quick reply!