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

[14.0][IMP] sign_oca: Link from partner to signer #19

Merged

Conversation

BernatPForgeFlow
Copy link
Member

Depends on: #8

@BernatPForgeFlow BernatPForgeFlow force-pushed the 14.0-sign_oca-partner_sign_request_link branch 2 times, most recently from c7bc98d to e38a013 Compare February 28, 2024 14:37
Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Code and functional review OK.

Won't really work until related is merged.

@BernatPForgeFlow BernatPForgeFlow force-pushed the 14.0-sign_oca-partner_sign_request_link branch from e38a013 to 043f77c Compare March 4, 2024 10:29
@pedrobaeza pedrobaeza added this to the 14.0 milestone Mar 4, 2024
name="action_show_signer_ids"
attrs="{'invisible': [('signer_count', '=', 0)]}"
icon="fa-pencil"
groups="sign_oca.sign_oca_group_user"
Copy link
Member

Choose a reason for hiding this comment

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

For this version, the groups should be put at view level, not element level, or you will have an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I understand what you mean. In version 14, groups can be added to buttons to not be shown. I have tested it and it works fine.

Copy link
Member

Choose a reason for hiding this comment

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

As said, if you enter in this view without having the permissions, it will give an access error. Please put the group at view level, not element level. Something like:

    <record id="view_partner_form" model="ir.ui.view">
        <field name="name">res.partner.form (in sign_oca)</field>
        <field name="model">res.partner</field>
        <field name="inherit_id" ref="base.view_partner_form" />
        <field name="groups_id" eval="[(4, ref('...')]" />
        <field name="arch" type="xml">
            <div name="button_box" position="inside">

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not agree that it would give access error. However, I change it since it seems right to me anyway.

Copy link
Member

Choose a reason for hiding this comment

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

If there are no access errors, it's because this model is readable for all users, but it's a good practice to do it this way because:

  • It avoids needing to access the model without need.
  • It avoids access errors when the model is not readable.

@BernatPForgeFlow BernatPForgeFlow force-pushed the 14.0-sign_oca-partner_sign_request_link branch 3 times, most recently from 1dcb2b6 to 8ac1c42 Compare March 6, 2024 09:26
Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Code and functional review OK.

I think sometimes there will be confusion of sign.request linked to a partner vs sign.request.signer, but it is "normal".

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge minor

@pedrobaeza
Copy link
Member

Oh, wait, there are conflicts, so they should be resolved before merging. @BernatPForgeFlow would you?

@BernatPForgeFlow BernatPForgeFlow force-pushed the 14.0-sign_oca-partner_sign_request_link branch from 8ac1c42 to 2b8206a Compare March 12, 2024 11:58
@BernatPForgeFlow
Copy link
Member Author

Oh, wait, there are conflicts, so they should be resolved before merging. @BernatPForgeFlow would you?

@pedrobaeza Can you merge now?

@pedrobaeza
Copy link
Member

/ocabot merge minor

@victoralmau to be cherry-pick in the v15 migration

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-19-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 420018e into OCA:14.0 Mar 12, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

4 participants