Skip to content

Conversation

macocianradu
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Sep 23, 2025

Pull request status dashboard

Choose a reason for hiding this comment

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

Nice 🥇

Comment on lines +22 to +25
if (index === -1) {
this.disabledElements.push(id);
return;
}

Choose a reason for hiding this comment

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

What your function does is:

  • If item is already disabled, remove it from the list
  • If item is not disabled add it to disabled list
    Thus, this if condition is actually half of your functionality. But when you return inside the block it looks like it's a corner case and for most records what will execute is this.disabledElements.splice(index, 1); which is not true.

In this case I would prefer to have

Suggested change
if (index === -1) {
this.disabledElements.push(id);
return;
}
if (index === -1) {
this.disabledElements.push(id);
}
else {
do other thing
}

so what your method does is clearer.
This is of course up for debate. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I guess it's a matter of preference.
I usually prefer to do early returns instead of else statements, so that if the function gets more complicated, I don't need to nest so much.
In this case it's probably the same.

Copy link

@cgun-odoo cgun-odoo left a comment

Choose a reason for hiding this comment

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

Nice, just keep in mind the ir.config_parameter will stay the same for every user (you can define it to be specific to company in a multi-company environment). But I was the one baiting you into using it instead of res_users so, 👍

A work around with only using ir.config_parameter is saving a json object as the parameter that has the shape {'user_id': disabledItems} then you can get the specific setting for every user

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.

3 participants