Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

[FIX] Readonly fields not being stored. #97

Closed

Conversation

richard-willdooit
Copy link
Contributor

If the wizard turns a field to readony because the combination
means it is unenterable now, then the value needs to be submitted
when the client attempts to write the record.

Since the Odoo client does not submit readonly values, then we
need to ensure we get that value by keeping an "invisible" equivalent
of the real value, which is never readonly.

Richard deMeester added 3 commits May 8, 2017 09:05
If the wizard turns a field to readony because the combination
means it is unenterable now, then the value needs to be submitted
when the client attempts to write the record.

Since the Odoo client does not submit readonly values, then we
need to ensure we get that value by keeping an "invisible" equivalent
of the real value, which is never readonly.
@codecov-io
Copy link

codecov-io commented May 8, 2017

Codecov Report

Merging #97 into 10.0 will increase coverage by 3.6%.
The diff coverage is 64.28%.

Impacted file tree graph

@@            Coverage Diff            @@
##             10.0      #97     +/-   ##
=========================================
+ Coverage   61.11%   64.71%   +3.6%     
=========================================
  Files          14       14             
  Lines        1368     1386     +18     
=========================================
+ Hits          836      897     +61     
+ Misses        532      489     -43
Impacted Files Coverage Δ
...configurator_wizard/wizard/product_configurator.py 46.5% <64.28%> (+13.25%) ⬆️
product_configurator/models/product.py 74.14% <0%> (+0.34%) ⬆️
product_configurator/models/product_config.py 76.84% <0%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89750da...f9f8670. Read the comment docs.

@PCatinean
Copy link
Contributor

@richard-willdooit when a field cannot be written because of the restriction rules then it means that the value should not be entered (invalid configuration). If we propagate the invalid value it will then likely return a ValidationError.

I think it was you that made a fix that turns values empty when not provided by the client. Is there some other scenario that I'm missing?

@richard-willdooit
Copy link
Contributor Author

@PCatinean You are correct, but the problem is the Odoo front end.

Consider this:

Attribute Model:

  • Basic
  • Advanced 1
  • Advanced 2

Attribute Engine

  • If Basic, no entry.
  • If Advanced 1 or Advanced 2, Engine can be Super or Turbo

Somebody configures a product:
Advanced 1 / Turbo

Then they choose to reconfigure.
Advanced 2 / Turbo

The wizard submits the value for Model as Advanced 2 because it has changed, but does not submit a value for Engine because it is unchanged. The backend correctly decides that unsubmitted values are unchanged, and uses the previously stored value of Turbo

Now, they choose to reconfigure.
Basic

The wizard is correct. It clears the options for engine, and makes it readonly. However, Odoo NEVER submits form values which are readonly, even if changed. So it submits the value for Model as Basic, but does not submit a value for Engine because it is readonly. The backend assumes that unsubmitted values are unchanged, and uses the previously stored value of Turbo, but this is invalid!

@richard-willdooit
Copy link
Contributor Author

@PCatinean

I have thought about another way to solve this, and it would actually make the readonly/required domains on the wizard easier to manage as well.

At the moment, add dynamic fields tries to create readonly/required "attrs" by pre-parsing the domains at that point. This has a couple of negatives:

  • More complex domains (Ands / Ors / and domains with multiple lines) are difficult to re-define in a way that is meaningful for attrs to parse.
  • Assumptions are made about "required" if the domain is not blank.

While the solution I have proposed here sorts out one lot of things (passing of readonly values back) an alternate way would be to have an invisible field associated with each value which determines the attributes as readonly/required.

The onchange would then set this value based on the calculated domains of the window. This means that the domain calculation could be as complex or as simple as needed and would only ever be in one place (at the moment, we have to implement the domain calcs, and then reverse engineer them to a degree to work with attrs).
The attrs would set from this and would be a very plain definition (single tuple).
The intention of code would be simpler to understand. This is important, too.
The tests would be simpler to write and understand.

I will try and look at this soon and propose an alternative.

In the meantime, can you decide on #84 and #87?

@PCatinean
Copy link
Contributor

@richard-willdooit sorry for the delayed response I'm really under pressure lately.

For readonly fields I was thinking a simple javascript function to remove all readonly attribute from all fields just as clicking the submit button form. Would be much less work this way.

I will do my best to review and integrate the mentioned PR's in the next version that I'm preparing.

@richard-willdooit
Copy link
Contributor Author

I think #104 is a better solution...

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

Successfully merging this pull request may close these issues.

3 participants