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

[17.0][MIG] web_m2x_options: Migration to 17 #2888

Merged
merged 67 commits into from
Jul 30, 2024

Conversation

manuelregidor
Copy link
Contributor

@manuelregidor manuelregidor commented Jul 22, 2024

There has been a wide refactoring work in this migration to adapt the module to the v17 environment and get a more compact code.

It also avoids checking whether certain modules are installed, which happens in other migrations proposals.

Some changes have been applied too:

  • The create option in the field's option dict is only available for m2o fields, as m2m fields already include this option in v17.
  • The m2o_dialog options have been removed as they don't seem to be used in v17.
  • The no_open_edit option has been removed as it makes no sense in v17, since edit mode is always active.
  • The no_color_picker options for m2m fields has been removed as the option canEditColor is avaiable in v17 base.

The commit history has been reduced.

Performance has been tested and no issues have been found.

@HaraldPanten
@ValentinVinagre

T-5977

Nicolas JEUDY and others added 4 commits July 22, 2024 17:05
- Add static/description/index.html

fix: use include instead of extend in js function inheritance.

fix: not overwriting the existing object references with the result of the include

fix: update name according to new module name.

fix: error when displaying many2many field without options defined.
[ADD] support 'no_open_edit' on many2one
[FIX] typos
@manuelregidor manuelregidor changed the title [WIP][17.0][MIG] web_m2x_options: MIgration to 17 [WIP][17.0][MIG] web_m2x_options: Migration to 17 Jul 22, 2024
@manuelregidor manuelregidor marked this pull request as ready for review July 23, 2024 06:15
@manuelregidor manuelregidor force-pushed the 17.0-mig-web_m2x_options branch 4 times, most recently from 54066aa to 15cad9e Compare July 23, 2024 09:23
@manuelregidor manuelregidor changed the title [WIP][17.0][MIG] web_m2x_options: Migration to 17 [17.0][MIG] web_m2x_options: Migration to 17 Jul 23, 2024
@HaraldPanten
Copy link

HaraldPanten commented Jul 23, 2024

@manuelregidor could you check @CRogos comments here --> #2847 about the commit history? I think it could be shorter.

You can add this information (that the commit history has been shortened) as well in the first message of your PR.

THX

@manuelregidor manuelregidor force-pushed the 17.0-mig-web_m2x_options branch 2 times, most recently from 5027c33 to 781138d Compare July 24, 2024 06:12
antespi and others added 13 commits July 24, 2024 08:15
…and make the module installable [MIG] adapt form.js to the new API. [FIX] Fix bug mentioned in pull OCA#262. [MIG] Make the module installable.
…th `options={'open': True}` it always open the first element with this fix it will open the right one
* [FIX][web_m2x_options] Fix Qweb templates.

They were trying to replace non-existing elements, and this was being logged to console:

    Can't find "a.oe_m2o_cm_button" when extending template FieldMany2One
    Can't find "span.badge" when extending template FieldMany2ManyTag

* Raise version correctly.

* [FIX] Make many2many_tags tag deletion work again

* Correct replacement of event

Instead of overwriting all events from upstream widget, better just overwrite the one you need and inherit the rest.
* fix unecessary calls

Do check_access_rights and disable_quick_create calls only once per field.
`web_m2x_options` is supposed to render fields in the context of a form or tree view, where domains can safely be applied because there's a main record that includes a context.

However, when installing along with `web_advanced_search_x2x`, they produce an incompatibility when a x2x field's domain is defined and depends on the current record's context, because a search view has no notion of a *current record*.

The fix is simple: try to do as usual, and if it fails, try without the field's domain.

Without this patch, an exception like this would be raised, i.e. when both addons are installed and you are trying to search project tasks by stage:

``` Error: NameError: name 'project_id' is not defined
http://localhost/web/static/lib/py.js/lib/py.js:370# Traceback:# Changes to be committed:
PY_ensurepy@http://localhost/web/static/lib/py.js/lib/py.js:370:19# modified: static/src/js/form.js
py.evaluate@http://localhost/web/static/lib/py.js/lib/py.js:1340:20#
py.evaluate@http://localhost/web/static/lib/py.js/lib/py.js:1397:35
py.evaluate@http://localhost/web/static/lib/py.js/lib/py.js:1409:34
py.eval@http://localhost/web/static/lib/py.js/lib/py.js:1453:16
eval_domains/<@http://localhost/web/static/src/js/framework/pyeval.js:869:39
_.forEach@http://localhost/web/static/lib/underscore/underscore.js:145:9
_.mixin/</_.prototype[name]@http://localhost/web/static/lib/underscore/underscore.js:1484:29
eval_domains@http://localhost/web/static/src/js/framework/pyeval.js:860:5
eval_domains/<@http://localhost/web/static/src/js/framework/pyeval.js:873:39
_.forEach@http://localhost/web/static/lib/underscore/underscore.js:145:9
_.mixin/</_.prototype[name]@http://localhost/web/static/lib/underscore/underscore.js:1484:29
eval_domains@http://localhost/web/static/src/js/framework/pyeval.js:860:5
eval_domains/<@http://localhost/web/static/src/js/framework/pyeval.js:873:39
_.forEach@http://localhost/web/static/lib/underscore/underscore.js:145:9
_.mixin/</_.prototype[name]@http://localhost/web/static/lib/underscore/underscore.js:1484:29
eval_domains@http://localhost/web/static/src/js/framework/pyeval.js:860:5
pyeval@http://localhost/web/static/src/js/framework/pyeval.js:977:16
eval_arg@http://localhost/web/static/src/js/framework/pyeval.js:988:16
ensure_evaluated@http://localhost/web/static/src/js/framework/pyeval.js:1011:21
call@http://localhost/web/static/src/js/framework/data_model.js:56:9
name_search@http://localhost/web/static/src/js/framework/data.js:537:16
get_search_result@http://localhost/web_m2x_options/static/src/js/form.js:130:50
OdooClass.extend/Class.include/</prototype[name]</<@http://localhost/web/static/src/js/framework/class.js:122:35
source@http://localhost/web/static/src/js/views/form_relational_widgets.js:271:17
_search@http://localhost/web/static/lib/jquery.ui/jquery-ui.js:6823:3
$.widget/</proxiedPrototype[prop]</<@http://localhost/web/static/lib/jquery.ui/jquery-ui.js:415:19
search@http://localhost/web/static/lib/jquery.ui/jquery-ui.js:6815:10
$.widget/</proxiedPrototype[prop]</<@http://localhost/web/static/lib/jquery.ui/jquery-ui.js:415:19
$.widget.bridge/$.fn[name]/<@http://localhost/web/static/lib/jquery.ui/jquery-ui.js:508:19
each@http://localhost/web/static/lib/jquery/jquery.js:383:49
each@http://localhost/web/static/lib/jquery/jquery.js:136:24
$.widget.bridge/$.fn[name]@http://localhost/web/static/lib/jquery.ui/jquery-ui.js:494:4
render_editable/<@http://localhost/web/static/src/js/views/form_relational_widgets.js:189:21
dispatch@http://localhost/web/static/lib/jquery/jquery.js:4640:50
add/elemData.handle@http://localhost/web/static/lib/jquery/jquery.js:4309:41
```
web_m2x_options: Fix usage for non-admins

web_m2x_options: Reduce rpc calls

web_m2x_options: Update manifest and readme
Currently translated at 91.7% (11 of 12 strings)

Translation: web-11.0/web-11.0-web_m2x_options
Translate-URL: https://translation.odoo-community.org/projects/web-11-0/web-11-0-web_m2x_options/pt_BR/
@manuelregidor
Copy link
Contributor Author

@HaraldPanten Commit history reduced

@pedrobaeza
Copy link
Member

/ocabot migration web_m2x_options

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Jul 24, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Jul 24, 2024
26 tasks
@HaraldPanten
Copy link

@CRogos could we continue the reviews here?

THX!

patch(many2OneField, {
m2o_options_props(props, attrs, options) {
const ir_options = session.web_m2x_options;
// Create

Choose a reason for hiding this comment

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

since you already refactored a lot, maybe you want to replicate my refactoring attempts I did in this alternative approach: 8eb54f7
There I essentially took the section comments and split the parts of m2o_options_props along these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback, but I don't think I'm following you. Could you reformulate your suggestion, please?

Choose a reason for hiding this comment

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

the m2o_options_props function is quite long and could be split into more reasonable sections. Each section is already introduced by a comment. So just follow this code smell rule: https://refactoring.guru/smells/comments
This would increase understandability of it as much as it makes it easier to adapt it when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mohs8421 done

@manuelregidor manuelregidor force-pushed the 17.0-mig-web_m2x_options branch 2 times, most recently from 37b997a to 90143dd Compare July 26, 2024 07:17
Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Functional review in runboat.

@pedrobaeza Could you review/merge?

As you suggested, we have considered/studied the performance and we didn't find any issue.

THX!

Copy link
Contributor

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

LGTM beside the confirm create dialog.

We disabled create... globally so this is not an issue for me anymore, but in the past this was very helpful to prevent creating entries accidently.

Comment on lines 29 to 30
<t t-name="web_m2x_options.Many2OneField.CreateConfirmationDialog" owl="1">
<Dialog title="title" size="'md'">
<div>
You are creating a new <strong t-esc="props.value" /> as a new <t
t-esc="props.name"
/>, are you sure it does not exist yet?
</div>
<t t-set-slot="footer">
<button class="btn btn-primary" t-on-click="onCreate">Create</button>
<button
class="btn btn-primary"
t-on-click="onCreateEdit"
>Create and Edit</button>
<button class="btn" t-on-click="() => props.close()">Discard</button>
</t>
</Dialog>
</t>
</templates>
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you removed the confirm create dialog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CRogos I removed it as I don't think it was working in v15 and v16 (or at least I couldn't make this option work). Are we sure they work in previous versions? Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right. Not working in v16 either.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-2888-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e7b3b7b into OCA:17.0 Jul 30, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 4d5d792. Thanks a lot for contributing to OCA. ❤️

@HaraldPanten HaraldPanten deleted the 17.0-mig-web_m2x_options branch July 30, 2024 08:00
@CRogos
Copy link
Contributor

CRogos commented Aug 1, 2024

@manuelregidor could you have a look into this #2901 ?

@manuelregidor
Copy link
Contributor Author

@CRogos As the feature was not working in v16, we decided that there was no point in keeping it in v17 as if it was properly working and that's why we removed it. At the moment, we are not planning to put the feature back I'm afraid, but we will keep you updated in case we do. Thank you.

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

Successfully merging this pull request may close these issues.