{"action":"closed","issue":{"url":"https://api.github.com/repos/OCA/openupgradelib/issues/46","repository_url":"https://api.github.com/repos/OCA/openupgradelib","labels_url":"https://api.github.com/repos/OCA/openupgradelib/issues/46/labels{/name}","comments_url":"https://api.github.com/repos/OCA/openupgradelib/issues/46/comments","events_url":"https://api.github.com/repos/OCA/openupgradelib/issues/46/events","html_url":"https://github.com/OCA/openupgradelib/issues/46","id":161098155,"number":46,"title":"Rationale behind PR series","user":{"login":"blaggacao","id":7548295,"avatar_url":"https://avatars.githubusercontent.com/u/7548295?v=3","gravatar_id":"","url":"https://api.github.com/users/blaggacao","html_url":"https://github.com/blaggacao","followers_url":"https://api.github.com/users/blaggacao/followers","following_url":"https://api.github.com/users/blaggacao/following{/other_user}","gists_url":"https://api.github.com/users/blaggacao/gists{/gist_id}","starred_url":"https://api.github.com/users/blaggacao/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/blaggacao/subscriptions","organizations_url":"https://api.github.com/users/blaggacao/orgs","repos_url":"https://api.github.com/users/blaggacao/repos","events_url":"https://api.github.com/users/blaggacao/events{/privacy}","received_events_url":"https://api.github.com/users/blaggacao/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2016-06-19T23:41:32Z","updated_at":"2016-12-14T17:26:08Z","closed_at":"2016-12-14T17:26:08Z","body":"As per the [docs](https://openupgradelib.readthedocs.io/en/latest/contributing.html#submit-feedback), I want to sum up the rationale behind my recent seris of PR, who mainly are still WIP.\n\nThe series comprises:\n#40 with it's sister #43 (concerning `get_legacy_columns()`)\n#38 (concerning wrapping of `Env`)\n#37 (concerning backward-compatibly shelling out map_value_orm to make it able to map m2m)\n#36 (small clean up helper for minor migrations, where no full blown migration is warranted)\n#35 (concerning xml mapping in map_value as a general improvement)\n#45 (concerning the ability of reliably copying m2m fields through the ORM)\n\nLet's start with the ultimate use case:\n\nAs a user of openupgradelib (not OpenUpgrade!), I want to use openupgradelib to help me standardize and optimize the following task:\n\n**Goal:**\nI want to update `account.tags` on `account.account` (m2m) based on detecting weather tags of `account.template` have been altered through an updated xml file during migration. (You might question, why I want to do that, but let's stay on topic, I have my reasons.)\n\n**Explanation:**\nIn order to be able to detect changes in the m2m field I need to know during post migration, what where the previous values. Therefore copying the m2m field preserving all it's relations is achieved by #45 . Clearly, I could manually manipulate the rel_table, but then again, I only can construct a little generic SQL script only for this specific use case and would need to throw it away afterwards. Additionally, manipulating rel_tables is not always intuitive, so this wrapper let's me focus on actual business logic while making future migrations scripts instead of getting stuck by the low level details of SQL queries of mid range complexity to the unexperienced SQLer. Also this task's would get complicated as it is not legit to assume that the target model of a m2m migration remains the same. So forcibly, one would need to keep old column \"as is\". But, Without salvaging the needed column through copying, it would get lost during module loading. On the simple table level it's easy, but on m2m it's slightly more complicated. That is why I built #45.\n\n**Details:**\nNow as the ORM does already quite a good job in housekeeping tracked fields and columns, despite in the case of m2m (alleged reason for #36), in order for ORM copies to be persisted across module updates, we need to mark them as \"manual\", which in some ORM routines is indicated by the `manual` flag in `ir_model_fields` yet in others simply by the `x_` prefix. Hence essence of #40 . As often times, constructing mapping dicts by database id's is not something where regex can really be helpful, contrary to xml_ids, #35 tries to solve this. Imagine the need to construct a 2000 lines mapping table based on db id's, I think it is self evident, why this can be quite a headache.\nAs both, mapping m2m and copying them can only be (afaik) conveniently done through the ORM, dividing functions into a `_orm` style and a `_sql` style became a natural conclusion. This is not directly related to the Rational of this issue, but includes several benefits. For example a clear separation throughout openupgradelib, when orm mechanisms are to be used and when the lower level pendant is necessary. Think about the ORM is being purposely fooled by the migration script or we need to handle stale columns/tables/values through direct SQL. Having both options side by side permits opting for the best of both worlds and generally increases the usefulness and generality of opernupgradelib. This being said, #38 tries to set the path and establish a common interface for future ORM style methods or refactoring of old ones.\n\n**Backward Compatibility:**\nAll this has been done with utmost care for backwards compatibility, so that exposed behaviour and known interface would not change, properly emitting deprecation-warnings at the appropriate points, while at the same time permitting the user taking advantage of the new subtleties, in case he needs to.\n\n**Documentation:**\nI can agree, that all this needs proper documentation, which has been included into the docstrings, where applicable. But I'm also motivated to undertake a first step towards improving [this](https://openupgradelib.readthedocs.io/en/latest/readme.html#features).\n\n**Further Implications / Thoughts:**\nIf you are inclined, you can read in the signature of this work the idea of deprecating methods in favor of others. I'm not aware what is/should be openupgradelib's stance on deprecation. I understand that compatibility is very important, yet I do not know if it _always_ wins against maintainability. In our case for example, the majority are or will be enterprise clients, so the main reason why we would commit to contributing to openupgradelib is in order to manage migration of own modules which we do routinely every week since beginning of this year. There is, no doubt, a stance in the code about a wider understanding of openupgradelib's raison d'etre. Some comments throughout the discussion warrant for the interpretation that openupgradelib is kind of a \"third wheel\" of OpenUpgrade, this is historically true indeed. Yet, I guess, there has been some deeper reason why lib was shelled out. And it's for this reason, I'm actually contributing, because it serves me very well in another, slightly different use case. This slightly different interpretation of openupgradelib together with a deprecation approach, could solve the compatibility problem also in another more flexible and dynamic way: by pinning down openupgradelib versions to specific OpenUpgrade Migrations.\nThis could help clean the codebase and ease maintenance while at the same time attracting more people to a dynamic and well conceived project (which as I stated in the mailing list, to my firm belief is an undervalued treasure).\n\nIf consens can be drawn on the general rational, I would be very happy to proceed with rigorous compliance with [PR-Guidlines](https://openupgradelib.readthedocs.io/en/latest/contributing.html#pull-request-guidelines) and get everything nice and clean.\n\nI hope, by this resume I can comply with the [relvant recommendations](https://openupgradelib.readthedocs.io/en/latest/contributing.html#submit-feedback) for proposing new features.\n\nThanks everyone for your time and effort, your collaboration and help, and your benevolent contribution to get this initiative proper.\n\nBest Regards,  David\n"}}
{"actor":{"display_login":"pedrobaeza"}}