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

Add onboarding panel for invoicing #24942

Merged
merged 10 commits into from Jul 20, 2018
Merged

Conversation

eco-odoo
Copy link
Contributor

@eco-odoo eco-odoo commented May 28, 2018

Add a config bar for invoicing app. After going through the config step, the system should be all set to send real invoices.

@eco-odoo eco-odoo added RD research & development, internal work Accounting labels May 28, 2018
@eco-odoo eco-odoo force-pushed the invoicing-config-bar-eco branch 22 times, most recently from 85c9114 to 8a64f94 Compare June 4, 2018 09:55
@eco-odoo eco-odoo requested a review from ged-odoo June 5, 2018 11:38
Copy link
Contributor

@ged-odoo ged-odoo left a comment

Choose a reason for hiding this comment

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

base -> web
ADD / IMP

@@ -2617,6 +2617,98 @@ var ImageSelection = AbstractField.extend({
this._setValue($(event.currentTarget).data('key'));
},
});
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

empty line above

Copy link
Contributor

Choose a reason for hiding this comment

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

add doc in javascript_reference

* @param {Object} option
* @param {Object} $target jQuery element
*/
_addPdfLink: function (option, $target) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename this

*/
var SelectionWithPreview = AbstractField.extend({
supportedFieldTypes: ['selection'],
events: _.extend({}, AbstractField.prototype.events, {
Copy link
Contributor

Choose a reason for hiding this comment

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

? put the image clickable ... investigate if it is possible to reuse existing widget

Copy link
Contributor

@ged-odoo ged-odoo left a comment

Choose a reason for hiding this comment

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

banner: add documentation

*
* @returns {Deferred}
*/
_renderBanner: function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

put this in controller

Copy link
Contributor

Choose a reason for hiding this comment

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

update (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

put this in controller

var def;
if (this.bannerRoute !== undefined && !this._requesting_banner) {
// solves concurrency problems
this._requesting_banner = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

use DropPrevious

args: [id],
}).then(function (action) {
if (action !== undefined) {
self.do_action(action, {
Copy link
Contributor

Choose a reason for hiding this comment

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

to confirm: this is builtin

Copy link
Contributor

@ged-odoo ged-odoo left a comment

Choose a reason for hiding this comment

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

add doc for banner_route in list

@@ -0,0 +1,482 @@
/// For the prototype pourpose only
.o_website_dashboard {
Copy link
Contributor

Choose a reason for hiding this comment

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

check if this can be lazy loaded

@include o-flex(0,1,auto);
padding-right: 12px;

&:not(:last-of-type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

to check: performance

@@ -53,6 +53,7 @@
<link rel="stylesheet" type="text/scss" href="/web/static/src/scss/rainbow.scss"/>
<link rel="stylesheet" type="text/scss" href="/web/static/src/scss/datepicker.scss"/>
<link rel="stylesheet" type="text/scss" href="/web/static/src/scss/banner.scss"/>
<link rel="stylesheet" type="text/scss" href="/web/static/src/scss/onboarding.scss"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

Copy link
Contributor

@tivisse tivisse left a comment

Choose a reason for hiding this comment

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

First review on the wizard.

<data>
<template id="onboarding_panel_payment_step" name="onboarding payment"
inherit_id="account.onboarding_invoice_panel">
<xpath expr="(//t[@t-call='web.onboarding_step'])[2]" position="after">
Copy link
Contributor

Choose a reason for hiding this comment

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

You could give an attribute name for each step to allow an easy inheritance ;)

@@ -609,6 +609,13 @@ def action_invoice_sent(self):
def message_post(self, **kwargs):
if self.env.context.get('mark_invoice_as_sent'):
self.filtered(lambda inv: not inv.sent).write({'sent': True})

# onboarding step marked as done when the email is sent.
if self.env.context.get('is_onboarding'):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should work without any context key. To investigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that onboarding_company_is is always the user company. So remove the context key and use self.env.user.company_id instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The corner case mentioned by al: user opens dialog, on another browser window switches company and then comes back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's already an issue on all the models in Odoo.

No need to play with context that way ;)

})
sample_invoice.action_invoice_open()

return sample_invoice
Copy link
Contributor

Choose a reason for hiding this comment

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

We still can register a payment or a credit note on the sample invoice ?

Is it intended ?

See: https://drive.google.com/file/d/1K6VyoKUZhnW43i-2L9ZDLNqa1LD5sqFY/view?usp=drivesdk

sample_user_type_id = self.env['account.account.type'].search(
[('type', '=', type_name)], limit=1)
sample_account = self.env['account.account'].create({
'name': 'Sample Account ({})'.format(code_name),
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 'Sample Account (%s)' % (code_name) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python 3 doc describes this formatting method as "old string formatting".

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but translators use to remove the {} by thinking that it's useless ;) Do as you wish in that case.

But by speaking about translations, please use _('') to make the whole string translatable ! ;)

data_fetched = False
cache = {}

def _get_company_id(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

These methods are not very usefull and could be removed.


return self.cache.get(key, '')

paypal_checked = fields.Boolean('Paypal', default=lambda self: self._get('paypal_checked'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be modular !

We propose to configure Paypal credentials even if the module is NOT installed ?

Define the related fields in the correct modules !

Copy link
Contributor

Choose a reason for hiding this comment

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

In the same idea, add a field onboarding_checked_paypal in payment_paypal instead of trying to hide that in a pseudo-cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that the modules may not be installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. That's the point. You can propose to install paypal on the original wizard, but not to configure the credentials if the module is not even installed.

return self.env['res.company'].browse(self._get_company_id())

def _get_manual_payment(self, env=None):
if env is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why trying to pass an environment ? You already have one on self .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is called 2 times, one time with a new env different from the current one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that, but why do you need to instantiate a new environment ? It shouldn't be the case. If you need to create a new environment because you just installed a module, then refresh the page instead.

self._install_module('account_payment')

# create a new env including the freshly installed module(s)
new_env = api.Environment(self.env.cr, self.env.uid,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should refresh the page instead of creating an new environment.


if len(modules_required) > 0 or self['manual_checked'] is True:
for module in modules_required:
self._install_module(module)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want to install modules on the fly ?

Could it impact the pricing ?

self._mark_step_done()
return {'type': 'ir.actions.act_window_close'}

def _mark_step_done(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename into something more accurate, like _action_onboarding_payment_step_done or something like this.

@eco-odoo eco-odoo force-pushed the invoicing-config-bar-eco branch 17 times, most recently from d3baeaa to 421a7df Compare July 20, 2018 06:46
eco-odoo added 10 commits July 20, 2018 11:59
Use a new view to choose the document template.
Remove layout fields from base company form.
Reword Edit Header -> Edit Layout
Set a default logo for a new company in data instead of demo data

task: 60668
It is useful to know when the stylesheet has been loaded in order to prevent displaying unstyled content.

CalendarView and the dashboard_graph widget (JournalDashboardGraph) use loadCSS through cssLibs / loadLibs so the tests now have to use createAsyncView instead of createView.
Add a banner_route attribute to AbstractView to allow fetching html from a
controller route and displaying it above the view.

The initial use case was the onboarding panels.

Supported views are the ones inheriting both AbstractView and AbstractController.
Before this, the deffered returned by _update was not taken into
account.
Add a base template file, html and stylesheet for the onboarding banners.
Add a step for company onboarding.

task: 60668
Add an onboarding panel for invoicing, it appears above the invoice
list.
After going through the steps the system should be all set to send real invoices.

In the payment module, add a payment acquirer step and add it to the invoice onboarding panel.

task: 60668
When the logo was not set and you tried to print a quotation, a modal form opened and offered you to edit the company data. You then had to save, close the modal and retry to print.
Now the quotation is printed without the logo.
Help users configure the system in order to send the first quotation to a customer.
This panel appears above the sale quotations list.

task: 1839195
Convert the current design of the account dashboard setup bar in order to uniformize the design of the onboarding panels.

task: 1858542
Add new onboarding steps: payment and theme selector. There are
initially used on the website_sale_dashboard onboarding panel.

task: 38876
@tivisse tivisse merged commit 19775de into odoo:master Jul 20, 2018
@tivisse tivisse deleted the invoicing-config-bar-eco branch July 20, 2018 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accounting RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants