shopinvader / odoo-shopinvader

Odoo Modules. Sorry Magento, Shopinvader is coming
GNU Affero General Public License v3.0
121 stars 104 forks source link

ObjectID #1000

Closed sebastienbeau closed 3 years ago

sebastienbeau commented 3 years ago

For historical reason we have an "ObjectID" field in the index (algolia specific need)

For front end dev having the id and objectID is confusing

So I propose

@thibaultrey @simahawk

thibaultrey commented 3 years ago

Ahhh great initiative, it's time to tackle this issue. So if I understand correctly, in a first step, depreciate objectId and Id would have the same value right kthe record id) ?

Do we have some api action who accepts only binding ID in parameters for now ?

sebastienbeau commented 3 years ago

We do not have api that use the "binding id" so we can remove it and then put the value of "objectID" into the key "id"

simahawk commented 3 years ago
simahawk commented 3 years ago
* here, we should remove completely `id` and/or `objectID` from the ir.export record

the only drawback of this would be that the json stored won't include the external ID but I think it's not a problem. Actually we avoid storing something useless...

sebastienbeau commented 3 years ago

It's ok for me.

I will do a PR in search engine/elastic/algolia and in shopinvader.

Did I remove totaly the key objectID (this will be a breaking change ? but V14 is a new version so we can add this in the changelog?)

Note for algolia this required field can be added automatically, for algolia case we will have the

simahawk commented 3 years ago

objectID is still required for algolia, no?

sebastienbeau commented 3 years ago

yes, but I will only push it for algolia and set the same value as id. So in the templating we always use the key id (for all search engine) and we only have this extra key objectId that we just do not use in the templating