Closed alexeirivera87 closed 9 months ago
Also there was an existing proposed migration #324 , make sure to check if someone has done the work already, or explicitly point out that you're superseeding the previous PR, please!
There is also another duplicated PR that proposes to migrate this module to v16. #265
@aleuffre @alexeirivera87 @mamcode Hi guys since there is 3 PR in total, what should we do now ?
Leave a review on all of them, or the one you think is closest to being merged, both in terms of code and in terms of the author being more active.
In terms of code I think this one is closest to being done, there are just a few changes to be made.
Happy to open a 4th PR myself, since I'd like to have this merged ASAP :rofl:
Ok let me check if it has been tested ok, I guess it is alreay done on production.
Hello @mcassuto @aleuffre @mamcode
Yes, I did check the existing PR before submitting this one:
About #324 , on this migration the only thing the author did was to change the module version (and even this is not correct), and to remove a function that was failing in the tests.
About #265 , here the only thing that was done was to change the version of the module.
Regards
Hello @mcassuto @aleuffre @mamcode
Yes, I did check the existing PR before submitting this one:
About #324 , on this migration the only thing the author did was to change the module version (and even this is not correct), and to remove a function that was failing in the tests.
About #265 , here the only thing that was done was to change the version of the module.
Regards
In that case, we usually write superseds #number of the MR
in the description, so we can keep track of such things. Thank you :pray:
Visually it works for me. Why did you remove the Regex on the test?
@alexeirivera87 if you could please address the changes that were requested, they're minor things and I think this is pretty close to getting merged.
if you don't have time, I'd like to pick it up myself (I'd preserve ownership of your commits) since I need this module merged as well.
Thank you for your contributions!
@alexeirivera87 can you check the issue with the REGEX?
Hello @aleuffre @etobella
I reverted the changes in unit tests, so assertRegex is used as before.
Now I have to check why the test failed in method test_next_maintenance_date_02
Regards
But the test seems unrelated to the regex. Can you check it¿
@etobella All tests passed.
Yes, the is related to dates. Some are failing when current date is January 30th or 31st.
regards
Wow!!! I see... that might happend every 4 years :laughing:
One option would be to use freezetime in order to avoid this. Can you try?
Hi, sorry for being late. Added freeze_time in the tests in v15 https://github.com/OCA/maintenance/pull/371 but I see there are v15 changes missing in the commit history, can you add it?
The problem with the last days of the month is that the last day of the following month is not used (in those that are generated).
@victoralmau @etobella
It should be ok now.
regards
/ocabot merge nobump
This PR looks fantastic, let's merge it! Prepared branch 16.0-ocabot-merge-pr-339-by-etobella-bump-nobump, awaiting test results.
Congratulations, your PR was merged at 4f5193f988318bf374e4b8fd12c20292b12b45c5. Thanks a lot for contributing to OCA. ❤️
/ocabot migraion maintenance_plan
Hi @etobella. Your command failed:
Invalid command: migraion
.
Ocabot commands
ocabot merge major|minor|patch|nobump
ocabot rebase
ocabot migration {MODULE_NAME}
More information
/ocabot migration maintenance_plan
Also there was an existing proposed migration #324 , make sure to check if someone has done the work already, or explicitly point out that you're superseeding the previous PR, please!