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

[8.0][ADD][website_snippet_mass_mailing_name] Snippet + name #66

Merged
merged 2 commits into from
Jun 13, 2016

Conversation

yajo
Copy link
Member

@yajo yajo commented May 30, 2016

Let you have the name field in the mass mailing subscription snippet.

When somebody tells his name, a partner is linked (if a match is found) or created.

Leave most logic to base module.

@Tecnativa relaunching OCA/website#201, related to OCA/crm#96.

<input
name="name"
class="js_subscribe_name form-control"
placeholder="your name..."/>
Copy link
Member

Choose a reason for hiding this comment

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

This should be configurable at snippet level (to include or not to include)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand you. The only purpose of this module is to include this input. If you don't want that, don't install it, no?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but you must be able to select the name granularity by snippet. In some occasions, you want the snippet to serve only for logued users, in other, for all introducing the name. I have another doubt now. If you are logged, the name of the user is respected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not right now.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so at least last point should be developed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@coveralls
Copy link

coveralls commented May 31, 2016

Coverage Status

Coverage increased (+0.3%) to 82.328% when pulling c3de0ee on Tecnativa:8.0-website_snippet_mass_mailing_partner into 19c1c4a on OCA:8.0.

@hbrunn hbrunn added this to the 8.0 milestone Jun 2, 2016
name = post.get("name")

# Update partner's name, to make it get updated in contact list later
Partner.search([("email", "=", email)], limit=1).write({"name": name})
Copy link
Member

Choose a reason for hiding this comment

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

check if you actually got a name

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, it doesn't matter, as write will act as dummy in that case

Copy link
Member Author

Choose a reason for hiding this comment

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

no, @hbrunn is right. We could be removing a name from a partner.

Copy link
Member

Choose a reason for hiding this comment

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

Remove this line as explained later.

@yajo
Copy link
Member Author

yajo commented Jun 2, 2016

Now that odoo/odoo#12265 is merged this module will be dramatically simplified.

@yajo yajo force-pushed the 8.0-website_snippet_mass_mailing_partner branch from c3de0ee to 10b9b07 Compare June 2, 2016 14:09
@yajo yajo changed the title [8.0][ADD][website_snippet_mass_mailing_partner] Snippet + name [8.0][ADD][website_snippet_mass_mailing_name] Snippet + name Jun 2, 2016
Let you have the name field in the mass mailing subscription snippet.

Leave most logic to base module.

Make name field autofilled if user data is available.

Add tour test.
@yajo yajo force-pushed the 8.0-website_snippet_mass_mailing_partner branch from 10b9b07 to cdf0d30 Compare June 2, 2016 14:12
@yajo
Copy link
Member Author

yajo commented Jun 2, 2016

Well, as said before, now the module is much simplified.

The JS calls are intercepted thanks to suggestions by @hbrunn.

No more dependency on mass_mailing_partner. Just install it if you want that extra functionality. Thus, it has been renamed accordingly.

Test does not work, probably due to the framework. Leaving out the last step and hope for the best for v9.
@coveralls
Copy link

coveralls commented Jun 2, 2016

Coverage Status

Coverage increased (+0.3%) to 82.055% when pulling 2f24857 on Tecnativa:8.0-website_snippet_mass_mailing_partner into 2ee5a68 on OCA:8.0.

@lasley
Copy link
Contributor

lasley commented Jun 11, 2016

👍 Code + runbot

"application": False,
"installable": True,
"depends": [
"mass_mailing",
Copy link
Member

Choose a reason for hiding this comment

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

website?

Copy link
Member Author

Choose a reason for hiding this comment

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

mass_mailing depends on website.

@rafaelbn
Copy link
Member

Tested in runbot 👍

@carlosdauden
Copy link
Contributor

carlosdauden commented Jun 13, 2016

👍

@rafaelbn rafaelbn merged commit 589e2ab into OCA:8.0 Jun 13, 2016
@yajo yajo deleted the 8.0-website_snippet_mass_mailing_partner branch June 13, 2016 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants