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

[15.0][ADD]website_sale_partner_firstname: New module to set lastname from portal #1007

Conversation

manuelregidor
Copy link
Contributor

New module to set and edit lastname from portal.
This field is mandatory when the contact is B2C.

@manuelregidor manuelregidor force-pushed the 15.0-add-website_contact_lastname branch from b57a18d to 2520c43 Compare August 23, 2023 08:32
@manuelregidor manuelregidor force-pushed the 15.0-add-website_contact_lastname branch 2 times, most recently from 4233698 to aee206b Compare September 14, 2023 06:08
@manuelregidor
Copy link
Contributor Author

@HaraldPanten

@HaraldPanten
Copy link

@manuelregidor Only some comments:

  • Could you add the version of the module in the PR title? (the commit message is OK).
  • Could you add some more information in the USAGE document? Such as how portal users should do that.
  • I'm not sure if oca_dependencies.txt has to be changed. Maybe @etobella could shed some light here. I don't really uderstand how does pip work here.

@etobella
Copy link
Member

No need to change oca-dependecies.txt, as it is generated automatically with pip 😉

@manuelregidor manuelregidor changed the title [ADD]website_contact_lastname: New module to set lastname from portal [15.0][ADD]website_contact_lastname: New module to set lastname from portal Sep 14, 2023
@manuelregidor manuelregidor force-pushed the 15.0-add-website_contact_lastname branch from aee206b to 804c1ed Compare September 14, 2023 14:31
@manuelregidor
Copy link
Contributor Author

@manuelregidor Only some comments:

  • Could you add the version of the module in the PR title? (the commit message is OK).
  • Could you add some more information in the USAGE document? Such as how portal users should do that.
  • I'm not sure if oca_dependencies.txt has to be changed. Maybe @etobella could shed some light here. I don't really uderstand how does pip work here.

Done

Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Functional review. Tested in runboat. Works as expected.

@chienandalu could you add a quick review? Any suggestion?

THX in advance.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Some comments in the formal part

"installable": True,
"depends": [
"auth_signup",
"website_account_fiscal_position_partner_type",
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this too specific for the task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I don't think I get what you mean in this point.

Copy link
Member

Choose a reason for hiding this comment

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

That this dependency is too specific and not everyone will be interested in pulling it to enjoy this new feature

Choose a reason for hiding this comment

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

then we would have to carry out 3 separate modules. Since we have made the extension for the secondname. I don't see any harm in indicating on the website whether the client is a company or an individual for the correct treatment of the fields (the addition of the dependency).

Choose a reason for hiding this comment

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

I agree. The module has been designed considering that a customer can be B2B or B2C. Considering this fiscal position type, the dependency is a must

website_contact_lastname/__manifest__.py Outdated Show resolved Hide resolved
website_contact_lastname/__manifest__.py Outdated Show resolved Hide resolved
website_contact_lastname/static/description/icon.png Outdated Show resolved Hide resolved
@manuelregidor manuelregidor force-pushed the 15.0-add-website_contact_lastname branch from 804c1ed to 00c93a3 Compare September 18, 2023 06:13
@manuelregidor manuelregidor changed the title [15.0][ADD]website_contact_lastname: New module to set lastname from portal [15.0][ADD]website_sale_partner_firstname: New module to set lastname from portal Sep 18, 2023
@manuelregidor manuelregidor force-pushed the 15.0-add-website_contact_lastname branch from 00c93a3 to fa494aa Compare September 18, 2023 07:12
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Oh, it looks like the module already existed!: OCA/e-commerce#804

@ValentinVinagre
Copy link

ValentinVinagre commented Sep 27, 2023

Oh, it looks like the module already existed!: OCA/e-commerce#804

We have several modules to upload:

  • website_sale_partner_firstname
  • website_sale_partner_second_lastname

both are with dependencies to the 'website_account_fiscal_position_partner_type' module because it makes sense to indicate whether certain fields are required or not depending on whether the user is a company or an individual user.
If we were to separate the modules we would have to make 2 more modules to hook the modules together:

  • website_sale_partner_firstname
  • website_sale_partner_second_lastname

with the 'website_account_fiscal_position_partner_type'.
What do you propose to do?
IMO: I think 4 modules is exaggerated. I would say that it is a good dependence on the type of client.

@pedrobaeza
Copy link
Member

The problem is that you are using the partner type in fiscal position to determine if individual or company, which is not correct. If core doesn't do that distinction, you should build another base module for that (although I think you can infer it through the input data).

@HaraldPanten HaraldPanten deleted the 15.0-add-website_contact_lastname branch September 27, 2023 15:59
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.

6 participants