[FIX] [16.0] stock_move_line_expiration_date_required: Remove required from field definition #1501
Conversation
Shide
force-pushed
the
16.0-remove_required_from_field_definition-stock_move_line_expiration_date_required
branch
from
b100c03 to
e1a3d60
Compare
Shide
force-pushed
the
16.0-remove_required_from_field_definition-stock_move_line_expiration_date_required
branch
from
e1a3d60 to
35d75d1
Compare
|
I think the ocabot merge will need to increment the verison of the module |
stock_move_line_expiration_date_required/migrations/16.0.1.0.1/post-migration.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Functional review.
LGTM, thank you @Shide.
https://www.loom.com/share/ec7071afa48644b69199d3d7bb8a537a?sid=22e04f0f-1975-4514-9126-db27f4847bac
…field definition Using the required on the field definition causes the database to set "not nullable" on expiry_date column on stock.move.line. Moved the required to the views.
Shide
force-pushed
the
16.0-remove_required_from_field_definition-stock_move_line_expiration_date_required
branch
from
35d75d1 to
335702d
Compare
There was a problem hiding this comment.
Functionaly 👍🏼
I think we should wait for @yajo or @rousseldenis approval to merge, technical review, thanks
| @@ -3,12 +3,15 @@ | |||
| { | |||
| "name": "Stock Move Line Expiration Date Required", | |||
| "Summary": "Expiration date is required to enter manually on Move Lines.", | |||
| "version": "16.0.1.0.1", | |||
| "version": "16.0.1.0.2", | |||
There was a problem hiding this comment.
suggestion(non-blocking): usually, you shouldn't use migration scripts in patch versions. That should indicate that the version bump should be major.
According to our guidelines, it'd be a major or minor bump.
Future-proofing the code, it's better to restrict migration scripts to major or minor versions too.
However, this one PR is a little bit crossing the line because it's really a bugfix. Thus, this is not required at this time IMHO. But at least there's some extra info for you to keep in mind for next times :)
|
/ocabot merge nobump |
|
Sorry @yajo you are not allowed to merge. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
|
/ocabot merge nobump |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at bf4885f. Thanks a lot for contributing to OCA. ❤️ |
yajo
deleted the
16.0-remove_required_from_field_definition-stock_move_line_expiration_date_required
branch
Using the required on the field definition causes the database to set "not nullable" on expiry_date column on stock.move.line.
Moved the required to the views.
MT-4224 @moduon @yajo @EmilioPascual @rafaelbn please review