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.

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

[16.0][ADD] sale_dms_field #375

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

agent-z28
Copy link

No description provided.

@agent-z28
Copy link
Author

@victoralmau
@bizzappdev

@agent-z28 agent-z28 force-pushed the 16.0-sale_dms_field-add-module branch from 13eb97c to cb52ad0 Compare October 16, 2024 11:53
Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Code and functional review OK.

Some comments:

  • According to the Guidelines change the PR title to [16.0][ADD] sale_dms_field: New module and the commit message to: [ADD] sale_dms_field: New module.
  • If in sales we set sequences with dates and we use for example S/%(year)s we will have an error similar to account_dms_field. I think that the most appropriate (in another PR) is to move https://github.com/OCA/dms/blob/16.0/account_dms_field/models/dms_field_template.py#L11 to dms_field (it is only necessary to replace / to - and then it can be removed from account_dms_field and it will work for this module (sale_dms_field) or any other.

@agent-z28 agent-z28 changed the title 16.0 sale dms field add module [16.0][ADD] sale_dms_field Oct 18, 2024
@agent-z28 agent-z28 force-pushed the 16.0-sale_dms_field-add-module branch from cb52ad0 to 255a821 Compare October 18, 2024 11:56
@agent-z28
Copy link
Author

@victoralmau i did the naming changes.
We can fix the mentioned issue in another PR ?

@victoralmau
Copy link
Member

The suggested change can be done in a new PR (it will be merged before this) or in this PR, but if it is done in this PR it should be the 1st commit (with a different message) and finally sale_dms_field

@agent-z28
Copy link
Author

The suggested change can be done in a new PR (it will be merged before this) or in this PR, but if it is done in this PR it should be the 1st commit (with a different message) and finally sale_dms_field

@victoralmau okay we will take care next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants