OCA / interface-github

Tools to interact with github from Odoo
GNU Affero General Public License v3.0
46 stars 77 forks source link

[11.0][FIX] github_connector keyerror 'blog' #34

Closed enriquemartin closed 5 years ago

enriquemartin commented 5 years ago

Description of the issue/feature this PR addresses:

On new database error when syncing new github organization (example: OCA). Error traceback: KeyError: 'blog'

imagen

Current behavior before PR:

Synchronization gets blog key on get_odoo_data_from_github but blog field does not exist. When it tries to match the key with model attribute we get key error.

Desired behavior after PR is merged:

Get blog key from github data and show it on form view.

pedrobaeza commented 5 years ago

I don't know how you get to that, but what we should do is to restrict the list of vals to valid fields, or directly ask before if the field exists.

elicoidal commented 5 years ago

Thanks for reporting but I tend to agree with @pedrobaeza otherwise, every time a new field appears in the framework sync we will have to update the code Can you dig a little bit more on a more long term solution?

enriquemartin commented 5 years ago

I don't know how you get to that, but what we should do is to restrict the list of vals to valid fields, or directly ask before if the field exists.

I just synced an organization (OCA) on a new database and the error happened.

Anyway I made some research and I saw that in previous version of this addon, there was a mapping of field-key. Here

In the migration this mapping has been replaced by a list of keys directly, except avatar_url. blog key contains data of the organization (web url) and it should be saved in an Odoo field; maybe the best option is to recover the mapping of the previous version or at least map the blog key to the corresponding field.

I see now that blog key should be mapped on website_url field as in the previous version of the addon. I make some changes and upload them.

pedrobaeza commented 5 years ago

@enriquemartin I think the change was done because sometimes other keys are not present, so the previous iterative process seems OK for me, but you only have to add a mapping dictionary with the GH label and the field name, that it will be the same for all except the blog one. Another option is to add an if with that specific condition.

enriquemartin commented 5 years ago

@pedrobaeza what I see proper is to create a method on abstract model abstract.github.model to get the mapping dict. Call it on get_odoo_data_from_github for better inheritance. Something like that?

    @api.model
    def get_conversion_dict(self):
        """Prepare function that map Github fields to Odoo fields"""
        return {
            'github_id_external': 'id',
            'github_url': 'html_url',
            'github_login': self.github_login_field(),
            'github_create_date': 'created_at',
            'github_write_date': 'updated_at',
        }

    @api.model
    def get_odoo_data_from_github(self, data):
        """Prepare function that map Github data to create in Odoo"""
        map_dict = self.get_conversion_dict()
        res = {}
        for k, v in map_dict.items():
            if hasattr(self, v):
                res.update({v: data[k]})
        res.update({'github_last_sync_date': fields.Datetime.now()})
        return res
pedrobaeza commented 5 years ago

Great, this seems a good idea.

pedrobaeza commented 5 years ago

Only a pylint error:

github_connector/models/github.py:158: [W8106(method-required-super), Github.create] Missing `super` call in "create" method.
pedrobaeza commented 5 years ago

Well, the error is there because you always must to call super in a inherit.

j-zaballa commented 5 years ago

Well, the error is there because you always must to call super in a inherit.

Yes. I just meant that the error is not related to this pull request and I don't know if PRs are supposed to address previous errors :)

pedrobaeza commented 5 years ago

@j-zaballa are you sure? The module is the same, and I would say is due to these changes.

enriquemartin commented 5 years ago

@pedrobaeza the error is on line 158 of github_connector/models/github.py file. This PR does not make any changes on that file; do I fix it anyway?

pedrobaeza commented 5 years ago

Yes, please :pray:

enriquemartin commented 5 years ago

@pedrobaeza Github class inherits from object and object class does not have a create method. That is, the class is defining the method, not overriding it. I guess that the tests check that there is a super call in all create methods of odoo models, but Github class is not a odoo model.

pedrobaeza commented 5 years ago

OK, I see, @moylop260 can we consider this as a bug in pylint-odoo?

pedrobaeza commented 5 years ago

So the error is another, Moi?