Skip to content

Conversation

@shashikala1998
Copy link
Contributor

@shashikala1998 shashikala1998 commented Apr 6, 2025

Why is this change needed?

Error messages related to the above fields (farm_detail_id, farm_land_rec_id and farmer_id)shown in the OpenSPP log or the database log.

How was the change implemented?

set fields as optional

New unit tests

Unit tests executed by the author

While installing the 'spp_farmer_registry_base' , No Error messages related to the above fields (farm_detail_id, farm_land_rec_id and farmer_id) should be shown in the OpenSPP log or the database log.

How to test manually

Related links

#788

@codecov
Copy link

codecov bot commented Apr 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.07%. Comparing base (aa7a32f) to head (5f87532).

Additional details and impacted files
@@            Coverage Diff             @@
##             17.0     #789      +/-   ##
==========================================
- Coverage   75.07%   75.07%   -0.01%     
==========================================
  Files         730      730              
  Lines       19285    19285              
  Branches     2400     2400              
==========================================
- Hits        14479    14478       -1     
  Misses       4298     4298              
- Partials      508      509       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 7, 2025

Copy link
Member

@reichie020212 reichie020212 left a comment

Choose a reason for hiding this comment

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

@shashikala1998 There's no changes in this PR and upon checking the two commits of this PR, it seems that you reverted the changes of your 1st commit.

If that is intended, it is better to close this PR.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2025
@kneckinator
Copy link
Contributor

Something is wrong with the PR, for sure. But I don't understand why it was closed as 1) there should be a difference and 2) the problem still persists in 17.0.

One commit 2160d07 fixes the issue, then the other commit 5f87532 undos the fix.

Is there a reason why there are two commits @shashikala1998 ?

I am reopening this PR.

@kneckinator kneckinator reopened this Apr 9, 2025
@shashikala1998
Copy link
Contributor Author

yes @kneckinator
There are some issues thats why I revert the commit and closed PR;

Initial Attempt: Removing required=True
I first tried to remove required=True from the fields (farm_detail_id, farm_land_rec_id, farmer_id), but discovered this isn't technically possible. When using _inherits, these relational fields must be required=True for Odoo's delegation inheritance mechanism to work properly.

Second Attempt: Moving to XML
I then attempted to move the field definitions to XML records instead of keeping them in the Python file. However, this didn't solve the issue because the _inherits mechanism still enforces the requirement at the ORM level.

Third Attempt: Post-Install Hook
After discussing with @gonzalesedwin1123 , we tried implementing a post-install hook. However, this approach also failed because:
The database schema updates happen before the post-install hook runs
The conflict between _inherits (which requires non-null values) and existing res.partner records (which have null values) occurs during the initial schema update
The post-install hook runs too late to prevent these schema conflicts

@kneckinator
Copy link
Contributor

If the issue is with the delegation inheritance mechanism, why is it used instead of something like this?
@gonzalesedwin1123 @shashikala1998

from odoo import models, fields, api
from odoo.exceptions import ValidationError

class Farm(models.Model):
    _inherit = "res.partner"

    farm_asset_id = fields.Many2one("spp.farm.asset", string="Farm Asset")
    farm_detail_id = fields.Many2one("spp.farm.details", ondelete="cascade", string="Farm Detail")
    farm_land_rec_id = fields.Many2one("spp.land.record", ondelete="cascade", string="Land Record")
    farmer_id = fields.Many2one("spp.farmer", ondelete="cascade", string="Farmer")

    @api.constrains('farm_detail_id', 'farm_land_rec_id', 'farmer_id')
    def _check_required_farm_fields(self):
        for record in self:
            if not record.farm_detail_id:
                raise ValidationError("Farm Detail is required for Farm records.")
            if not record.farm_land_rec_id:
                raise ValidationError("Land Record is required for Farm records.")
            if not record.farmer_id:
                raise ValidationError("Farmer is required for Farm records.")

    @api.model
    def create(self, vals):
        # Ensure that when a new Farm is created, the related res.partner is also created
        # (This is usually handled automatically by _inherits, but good to be aware)
        res = super().create(vals)
        return res

@kneckinator
Copy link
Contributor

Also note that what the required=True aims to prevent is the following operation on the database side:

devel=# select id, farmer_id from res_partner where farmer_id is not null limit 1;
 id | farmer_id
----+-----------
 68 |        18
(1 row)

devel=# update res_partner set farmer_id = null where id = 68;
UPDATE 1

If the constraint "required = True" had been properly applied, the database would have prevented me from doing the above.

@gonzalesedwin1123
Copy link
Member

@kneckinator I suggest @shashikala1998 can work on re-creating the not null constraint in the post_install method of the module but the existing records in res.partner before the module is installed must be populated first with the associated rows to resolve the failing of the constraint creation. By default the "Administrator" and "Default Company" are created upon odoo installation.

@shashikala1998
Copy link
Contributor Author

post_install method

I have tried post_install method but its not working

@kneckinator kneckinator changed the title [FIX] Remove unnessary mandory options [FIX] Remove unnessary mandatory options Apr 22, 2025
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.

spp_farmer is trying to make guarantees it cannot make

5 participants