The problem occurred in form views with a many2one field, when the
user reproduced the following steps:
- type some text in the many2one
- click on 'Create ...' in the many2one dropdown to quick create
(name_create RPC) the record
- before the name_create returns, click on 'Save'.
When this happened, the record was saved before the many2one was
updated with its new value. So the record was incorrectly saved,
and the view was again dirty as soon as the name_create returned,
even if it was in 'readonly'. So after saving, a 'Changes will be
discarded' dialog opened when the user tried to leave the form.
We fix this issue by making the controller wait for the name_create
to return, and for the many2one to update its value, before saving.
This issue could also occur in list views.
The 'Add an item' button of one2many editable lists is deactivated
once clicked (since rev. f9a1241) to prevent concurrent record
creation. It is re-activated when the record has been created.
Unfortunately, it might happen that the record is actually not
created, so in this case the button is never re-activated.
This happens for example on the customer invoice form view, if
the user tries to add a line before selecting a partner.
This function should return a deferred, as indicated in the
docstring. However, in some cases, it doesn't. It then crashed
as the FieldX2Many tries to call done() on undefined.
This happens for instance on the customer invoice form view:
create a new record, select a partner, add a line, select a
product, directly click on the invoice date field (this will
trigger an onchange which will totally override the value of the
one2many). The FielX2Many calls setRowMode to switch the edited
row back to readonly, but this row doesn't exist anymore as the
one2many value has been overriden by the onchange, so setRowMode
wrongly returns nothing, and it crashes.
Previously, when an update operation was performed, the list of
ids in the x2many field was emptied beforehand.
It kinda worked in lots of cases where one would expect it to
fail because in thoses cases the widgets send link_to commands
along with the update one, thus re-adding the removed line.
Now the behavior is more consistent accross the board. We start
with the current list of ids and apply the requested changes.
When the user writes something in a Many2One input, after a short
delay, a name_search RPC is done with the value written in the
input, and results are displayed in a dropdown. Then, if the user
clicks on the input, the dropdown closes. If he clicks again, it
should do again a name_search RPC with the value of the input.
Before this rev., it was doing it with the empty string.
Clicking multiple times on 'Create' (in main list views) or 'Add an
item' (in o2m lists) might create invalid rows (i.e. rows with
required fields unset).
Creating a new record requires two sequential RPCs: a default_get
and an onchange (only if there is an onchange of one of the
record's field). When clicking twice to create a new record, that
sequence of RPCs is done twice in parallel, after unselecting the
current row (i.e. saving it if it is valid). However, in both
cases, there is no line to unselect yet (as both operations are
done basically simultaneously). When the first onchange returns,
a first new record is added to the list. When the second returns,
the second one is added and takes the edition, leaving the first
one, invalid, in the list.
This rev. ensures this doesn't happen by preventing concurrent
record creation.
... before saving a line.
Suppose that we have an editable list with one required field
(e.g. many2one) with an onchange. We add a record and select a value
for the many2one. Before the onchange returns, we click again on
'Add an item'. If we don't wait for the onchange to return before
unselecting the row (i.e. before saving it), a confirm dialog asking
if the changes can be discarded opens, because the line isn't valid
yet. This could easily be reproduced on the sale_order form view,
with some throttling.
This rev. ensures that we wait for the onchange to return before
saving the line, and thus before creating a new line.
In a slightly too naive way, we apparently tried to improve the
parse_float method while rewriting the new views. This was done by
using a regexp to match all occurences of a thousand separator.
The way the regexp was built was fine, until the thousand separator is a
'.', which, if used as a regexp, matches all characters. Also, sadly,
'.' is not so rare as a thousand separator...
Suppose that there is a many2many list (non editable) in a form
view. When one of the many2many subrecords is edited (from the
form view in a dialog), it is saved directly when the user clicks
on 'Save' in the dialog.
Before this rev., if the user clicked on 'Save' in the main form
view after editing a m2m subrecord, a 'write' RPC was done, whereas
there were nothing left to save (as the subrecord had already been
saved).
Suppose that we have a form view with a one2many list (non
editable) containing a record. In edit mode, the user clicks on
that record to edit it in a form view (in a dialog). In this form
view, there is an x2many field that isn't in the list. Before this
rev., it crashed when the user clicked on 'Save' in the dialog
after editing the value of the x2many.
This was because the basic model didn't use the correct fieldsInfo
in that case (it use the fieldsInfo of the list view instead of the
one of the form view, and the x2many field isn't in the list).
Also ensures that the record is correctly saved even if editing the
x2many is the only change done, as this wasn't the case before.
Before doing an onchange in a x2many, the basic model tries to evaluate
if the line is valid or not. In that computation, we used the
field.required property instead of using the required modifier. This
commit had to move the modifier handling from the BasicRenderer to the
BasicModel, in order to use it in the _isX2ManyValid method.
Before this commit, it was only possible to click on input to
change the value of the many2many_checkboxes widget.
For better consistency, we must allow to click on labels.
The motivation for this commit was to fix a wrong evaluation context for
x2m fields: it was using a list of res_id, which may contain virtual
ids.
For example, it could be seen in the form view for journal entries:
adding the first line (field line_ids) was fine, but clicking on 'Add an
item' a second time caused a crash. The reason was that the line_ids
was explicitely added to the context in the view, and the second time we
add a line, the web client sent a list looking like this [35,
'virtual_43'] to the server, in the nameget context.
The proper behaviour is to send a list of commands for those fields.
This means that we had to do some non trivial changes in the
getEvalContext method: we stop using the get method with raw: true. It
was nice to use the same method, but the eval context is actually
different that the values returned by the get method. That was the
motivation to introduce the 'noUnsetNumeric' flag, but now, we simply
create another method and let each method do its own work.
TLDR: eval contexts are hard...
It is not really easy to handle discard operations. We need to handle
the discard in editable lists, and in x2manys. This is not exactly the
same code, but most of this was done by moving the discard method from
the formcontroller to the basic controller.
With this commit, the user can now discard the changes for the current
record by pressing ESC. If the record is new, it will remove it, if
not, it will ask for confirmation before.
The issue that we fix in this commit is the following: open a form view
with a o2m with one required field, and the o2m is editable. When
adding a new line, it is invalid (since the field with required), but
when pressing ENTER, the editable list tries to navigate away, which
will discard the current line (in the unselectRow method), before
completing the move_to_next_line. So, at this point, the list editable
create a new (empty) record, and we are back to square one, but with a
visible flicker and no information on what happened.
To fix this issue, we simply check that the renderer can be saved when
moving to next line. If it is not (the line is invalid), then we simply
abort the move.
Notes:
* this behaviour worked fine in list view, because the canBeSaved check
is made by the saveRecord method in listController
* it looks like the protection should be put into unselectRow, but this is
not true (at least, in the current state): in some cases, calling
unselectRow with invalid state should result in discarding the current row
(in a new line in a o2m, when the record is not dirty, for example), and
the list editable renderer does not know if this is the case or not.
However, if the unselect action comes from a navigation event, then we want
to avoid discarding the current row.
When a many2one (in an editable list) with an onchange was edited, it
could happen that a modal window asking for confirmation of the creation
of a record was opened when it should not. Here is what actually could
happen:
* user focus a many2one
* user type something
* user press ENTER (keydown + keyup)
* user press ENTER again
* the onchange comes back
* modal window is open
Because of the onchange, the _reset method was called after the
_onInputKeyup method. So, the m2o_value was not equal to the
$input.val() and the 'floating' flag was set to true (after the first ENTER).
The second ENTER caused a unselectRow in the editable list, so the input
was focusouted, and the modal is set. This is basically caused by the
asynchronous behaviour of the onchange.
We simply fix the problem by setting the floating flag back to false in
the keydown method.
When a many2one (in an editable list) with an onchange was edited, it
could happen that a modal window asking for confirmation of the creation
of a record was opened when it should not. Here is what actually could
happen:
* user focus a many2one
* user type something
* user press ENTER (keydown + keyup)
* the onchange comes back
* user press ENTER again
* modal window is open
Because of the onchange, the _reset method was called after the
_onInputKeyup method. So, the m2o_value was not equal to the $input.val() and
the 'floating' flag was set to true, which means that the _onInputFocusout
method (called indirectly by the listeditablerenderer when unselecting the row)
was forced to open a confirm dialog.
We simply fix the problem by setting the floating flag back to false in the
reset method.
Before this commit, when the user write some text in a many2one, then
press ENTER, the key event will trigger a navigation move, which will
validate the field, and move the focus to the next line. The same issue
happens when pressing TAB.
With this commit, we simply stop the propagation, and preventdefault the
event that cause the selection.
Before this commit, when editing a many2one (with an onchange) by
clicking on the small form button on the right, then changing something,
then saving the modal dialog, no onchange was triggered, because,
technically, the value of the many2one was not changed, only the values
from the record pointed by the many2one were.
The same remarks applies to many2manys.
To see this issue in odoo, you can open a product (technically, a
product.template) from the 'Catalog' menu in sales, then edit it, then
click on the small form view icon next to Internal Category. This is a
many2one (categ_id) with an onchange. In the form view dialog, change
something, for example costing method, then save. The res_id of the
internal category was not changed, but an onchange should still be
triggered.
When a SelectCreateDialog was opened with a given domain, this
domain was completely lost if the user wrote a new domain (or
groupby) in the searchview, whereas they should be concatenated.
Suppose that there is a one2many list in a form view containing
a field F, displayed with no widget, and that F is also displayed
in the one2many form view (opened when clicking on a record of the
list), but that in this form view, F is displayed with a widget
requiring specialData (additional data to fetch alongside the
record), e.g. widget="domain".
Before this rev., it crashed when this happened because those
specialData weren't loaded when opening the related record.
Credit goes to @pga-odoo for reporting the bug and reproducing it
in a test.
DebounceFields debounce the function that notify field changes to
their environment (using _.debounce). It may happen that this
function is called once the field widget is destroyed (e.g.
type something in a char field and quickly click on Save, the
function is called directly by _commitChanges, but the debounced
call is still pending). It may be a problem if the subsequent
code does something like this.getSession().some_key, as if the
widget is destroyed, it has no parent that can provide it the
session, and thus it crashes.
Also rename attribute _debouncedStarted into _isDirty as it makes
much more sense.
Passing multiple ids wasn't correctly handled as the argument `record_id`
was wrapped in a list of list, which is not hashable.
The wrapping is now handled by the caller.
Before this commit, when creating a new record, we only looked at the
main fields to determine if a call to onchange was needed. This was not
enough, since sub fields (fields in a one2many for example) can have an
onchange.
This was an issue only for new records with onchanges in sub fields, but
no onchange on main fields. For example, the merge contact wizard.
To solve this issue, we had to propagate the current view type from the
field to the basic model, because otherwise, the main viewtype was used
to compute the onchangeSpec, and that is not always correct.
Before this rev., the FieldRadio didn't work on selection fields
with keys like "0", "1"... i.e. strings that can be converted to
numerical values. This was due to an automatic conversion of those
keys to their numerical value by JQuery, so when the user changed
the value, an unknow key was sent to the server (e.g. 1 instead of
"1"), causing a crash.
This rev. changes the logic a bit so that the keys are no more
written in the DOM, but their index in the selection are.
The server doesn't make a distinction between an unset selection
field, and a value 0 (integer). So when 0 is a valid key for the
field, the server returns false as value if the field is unset, or
if its value is 0, which is not really convenient.
As a consequence, when the value is 0, it appears as unset in the
interface. To circumvent this, we automatically converts false
value of selection fields to 0, if 0 is a key of the selection.
Many2many default value can be a list of ids, and in that case, we did
not properly load data.
It could be seen by selecting a few contacts (in list view), then
opening the 'Merge contact' wizard in the sidebar (action menu). Before
this commit, the wizard did show a list with some empty rows (for the
field partner_ids).
In the xml definition of views, a 'currency_field' can be specified
in the node options:
<field name="amount" widget='monetary'
options="{'currency_field': 'currency_id'}"/>
It indicates which field the monetary widget has to use to
determine the currency to display.
Before this rev., the widget didn't look for the 'currency_field'
in the options, but directly in the node attrs, so this simply
didn't work. This was for example the case on the crm.lead kanban
and form views.
This rev. removes some html injections issues:
- in char and text field widgets
- in phone field widget in mobile
- in list views (when no widget is instantiated)
Previously, the string attribute of the field node was ignored
in the title of the form dialog and it always displayed the
string value of the python field definition instead, which is
a default value instead.
Additionally, I took this opportunity to add tests for other
similar dialog titles which actually worked already with the
string attribute of the field node but were not tested yet.
Previously, when a checkbox was re-rendered, it was losing focus
since it was removed and re-created. Now, if the checkbox itself
is the origin point of the re-rendering, we give the focus to the
newly re-rendered checkbox, resulting in a seamless experience
just as if the checkbox was never removed and re-rendered.
Before this commit, it was not possible to see the difference between an
unset float field (with a 'false' value) and a set field (for ex, with
value 0)
This was due to the fact that the formatFloat/formatMonetary functions
format false values as the number 0, unlike many other formatting
functions.
... in x2many editable list views.
Field widgets can detect that they are at the origin of a
field_changed event, so that they don't re-render themselves when
they are being edited.
This didn't work for fields inside x2many list views. This wasn't
a big problem for most of the fields, because the cursor position
was automatically reset after a re-rendering, but it wasn't
working for monetary fields that empty their $el before each
re-rendering.
This rev. fixes the issue by making the field widgets inside
x2many list able to detect that they are the origin of a change,
and thus removes the now useless code that restore the cursor
position.
The 'get' method in the basic model is called very often (too often, but
that is another issue). For a large list with many columns, it can be
quickly be called more than 10k times.
In that function, we had to protect our integrity by performing a deep
clone with $.extend, so noone else can be allowed to change something in
our internal structure. This is good in theory, but not so good in
practice, as $.extend is a really expensive operation.
Also, we try to have the smallest external API by deleting useless keys.
But instead of keeping a blacklist of private keys, it is simpler to
have a whitelist of public keys.
So, what we do is to explicitely construct the object returned by key,
since we know the types of every values involved. This is more
explicit, and at the same time, much faster.
Also, we propagate the options for elements of type list. This is why I
had to change the isFieldSet method.
In a benchmark I just ran, loading and rendering a large list (200 rows,
7 columns) went from about 2.7s to 0.7s.
We still do way too much work, but at least it is much faster...
This rev. fixes 2 issues about many2many fields displayed with
an editable list view, occuring when the user edited some values
in the list.
First, the many2many field was re-rendered each time one of its
sub-widgets triggered a 'field_changed' event, and thus the row
the user was editing was no more in edition and the focused field
was no more focused. This was quite annoying.
Second, when the user saved changes made in the editable list view,
only a command 6 (REPLACE_ALL) was sent, so updates of subrecords
were never saved.
Before this rev., if you opened a form view with, e.g. a float
field, that is unset, and then click on edit, then save, it
actually saved the new value for the float field (being 0),
whereas it shouldn't do any RPC as nothing changed.
This is because the former code couldn't detect if the value "0"
in the input is there because the field is still unset or if the
user simply set it to 0.
Before this rev., the function that parses monetary fields (i.e.
the one that converts the string value of the input into a
numerical value) totally ignored the sign of the value.
Actually, parsing a monetary field is exactly the same as parsing
a float field, so we simply use the same function.
When a record is added in an x2many field with an onchange, the
onchange is only triggered once the new record is valid (i.e. when
all its required fields have a correct value).
However, there was an issue in the code checking the validity of
a record, as it checked, for each required field, that its value
wasn't falsy (!value). This is incorrect for several type of
fields:
- numerical: a value of 0 is correct
- boolean: should be always considered as correct
- x2m: their value is a dataPoint, so it is never falsy, what
should be check is the number of records inside the dataPoint.
This was an issue for example in the sale.order form view, as the
onchange wasn't triggered when an order line was added (with a set
product), so the total amount wasn't updated correctly.
Since the new views merge, a lot of design elements were broken. This
was particularly impacting the fields in the editable list view; indeed
the editable list view is not using an inline form view anymore so the
fields in the list were not properly styled as the LESS was still
defined assuming the form view environment (for example the invalid
fields were red for o2m fields in form view but not in editable list
view even though they got the right CSS class).
This commit refactor the LESS following these rules:
- No more division of non-layout and layout rules. Dividing LESS rules
in x_layout.less and x.less was a mistake. Many rules can be
considered to be layout and not layout at the same time, developers
always have to switch from one file part to the other, many CSS
selectors (and rules!) are duplicated for nothing, ...
- Field style is extracted from x_view.less and put in the new
fields.less file. As before, the fields_extra.less will contain the
rules specific to community so that the enterprise repo can override
those by replacing the whole file.
- Many classes have been renamed so that o_form_x_y becomes o_x_y as
many classes can now be applied outside of form view. These classes
should not be used in templates anyway.
The commit also changes the DOM of fields so that it is more minimalist
(no useless parent div, etc).
Input elements are not automatically styled anymore, they have to get
the o_input class explicitely. This allows to fix lots of small style
bugs of previous versions (required monetary field had not the proper
style, readonly m2m tags appeared as editable, ...). This also improves
the LESS code.
The editable list view should also completely stop flickering on chrome
and firefox.
The commit also removes the orange outline on list view dirty cells.
The commit also removes deprecated static xml, LESS and other code.
Notice there are still styles to restore/fix and LESS to improve.
When we unselect a line in a one2many list, we ask for all fields to
commit their changes (because many fields are debounced, so we may not
know their current last value). This is good, except that it could
trigger an extra field change event, which may in turn trigger a second
onchange.
We solve this issue by improving the _setValue method and delegating the
comparison to a new _isSameValue method.
It is particularly relevant for field types such as integer, which are not
stored as a string.
Suppose that there is a many2many (list) field in a form view,
and there is a many2many (e.g. many2many_tags) field inside the
list.
In edit mode, if the user adds a record in the relation, this
record and its relational data should be fetched.
Before this rev., the relational data wasn't fetched, so the
many2many_tags couldn't be displayed correctly.
When a record is added in an x2many field with an onchange, the
onchange is only triggered once the new record is valid (i.e. when
all its required fields have a correct value).
However, there was an issue in the code checking the validity of
a record, as it iterated on all known fields of the model (instead
of those in the view), so if there were required fields that
weren't, the record was always invalid (as those fields obviously
never had a value), and thus the onchange was never done.
This wasn't a problem with embedded views as in this case, the
known fields are those in the view. But when the view isn't
embedded, all fields of the related model are known, even if only
some of them are in the view.
This was the case for example in the stock.picking form view, when
adding a line in the stock.move o2m: the onchange wasn't done and
thus, the schedule date wasn't correctly set.
qsm-odoo