OCA / oca-github-bot

The GitHub Bot of the Odoo Community Association (OCA)
MIT License
40 stars 57 forks source link

[IMP] make _set_lines_issue working on empty issue #190

Closed legalsylvain closed 2 years ago

legalsylvain commented 2 years ago

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

CC : @pedrobaeza, @sbidoul

thanks for your review.

HaraldPanten commented 2 years ago

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 commented 2 years ago

Hi @HaraldPanten

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

legalsylvain commented 2 years ago

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

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

1)

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

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

3)

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.

legalsylvain commented 2 years ago

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 commented 2 years ago

Question : there is pending 183.bugfix file that has not disappeared even if the PR https://github.com/OCA/oca-github-bot/pull/183 is merged. Is it normal ?

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

legalsylvain commented 2 years ago

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

Thanks ! I thought it was during merge process.

HaraldPanten commented 2 years ago

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!