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 missing field attributes #36

Merged
merged 1 commit into from
Aug 5, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jul 21, 2015

No description provided.

@bwrsandman
Copy link

Why are these missing? The field is related. Please explain the change.

@ghost
Copy link
Author

ghost commented Jul 21, 2015

@bwrsandman you can make the test
1 - go to the list of travel.travel
2 - export the data including the field travel_state of the related traveller
you will get an error saying the field has no attribute selection

@ghost
Copy link
Author

ghost commented Jul 21, 2015

@bwrsandman in openerp/osv/orm.py ? in __export_row row 1173
I know this does not seem much logic, and this could be fixed in the orm. I guess this problem is fixed in v8.

@bwrsandman
Copy link

Ok, I see the error. That's an odoo bug.

One thing to fix, though. Don't duplicate the states, just factor them into a class-less function in travel.py

def _get_states(self, cr, uid, ids=None, context=None):
    return [
        ('draft', _('Draft')),
        ('open', _('Saved')),
        ('booking', _('In Reservation')),
        ('reserved', _('Reserved')),
        ('confirmed', _('Confirmed')),
        ('done', _('Closed')),
    ]

Then use it like this:

from openerp.addons.travel.travel import _get_states
# ...
_columns = {
    # ...
    'state': fields.related(
        selection=lambda *a, **kw: _get_states(*a, **kw),
    ),
    # ...
}

Also factor the original so the states are only defined once.

@ghost
Copy link
Author

ghost commented Jul 21, 2015

@bwrsandman can you add wip label ?

@ghost
Copy link
Author

ghost commented Jul 22, 2015

@bwrsandman I fixed the issue. You may remove the wip label. Thanks

('reserved', _('Reserved')),
('confirmed', _('Confirmed')),
('done', _('Closed')),
]

Choose a reason for hiding this comment

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

You're going to need to regenerate the pot files because these terms are now code

@ghost ghost force-pushed the 7.0-fix_field_attributes branch from 072282c to 950375a Compare July 22, 2015 13:48
@@ -25,6 +25,17 @@
from .res_config import get_basic_passenger_limit


def _get_travel_states(self, cr, uid, ids=None, context=None):
Copy link
Member

Choose a reason for hiding this comment

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

Include a reference that this is due to a bug, and even better if there's an open issue about it and it's linked here

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza This is related to a real bug, and there is an issue. The issue is internal to Savoir-faire Linux, not public in the web. I don't see why a comment/reference is required here. If you write a function field of type selection, you must supply a list of selections. This is standard Odoo/Openerp behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I mean the bug in ORM that doesn't resolve this correctly, as mentioned by @bwrsandman. This is only a workaround as ORM is not populating correctly the selection list.

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza This is not a bug in the ORM. The ORM works as designed, with its limitations.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I don't think so. If you put a related field, it's not logical to need again the selection list, but nevertheless, it's not a blocking one. It's only for tracing the problem and if in the future is solved (I think so that it's in v8), you can remove this part

Choose a reason for hiding this comment

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

@pedrobaeza @dufresnedavid showed me the bug, it's hard to believe but it is true what he says.

@pedrobaeza
Copy link
Member

👍

@houssine78
Copy link
Sponsor

👍 review no test

@max3903 max3903 added this to the 7.0 milestone Aug 5, 2015
@ghost
Copy link
Author

ghost commented Aug 5, 2015

@max3903 Can you merge this PR please ?

max3903 pushed a commit that referenced this pull request Aug 5, 2015
@max3903 max3903 merged commit ac294ab into OCA:7.0 Aug 5, 2015
@max3903 max3903 deleted the 7.0-fix_field_attributes branch August 5, 2015 15:35
@bwrsandman
Copy link

@max3903 @dufresnedavid This repo has been failing since this was merged, please fix before merging anythin else.

@ghost
Copy link
Author

ghost commented Aug 13, 2015

@bwrsandman the travis build was ok in this PR. Why do you think it is related to the current problem ? I would say #38 is the cause of the problem.

@bwrsandman
Copy link

This was merged before #38 and you can see the build starts failing at this merge:

https://travis-ci.org/OCA/vertical-travel/builds

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

Successfully merging this pull request may close these issues.

5 participants