[16.0][IMP] sign_oca: enable signing from portal

[kobros-tech]

adding a new feature by enabling portal users from signing their documents from portal in addition to signing it from emails too.
same as #78 but for version 16.0

[OCA-git-bot]

Hi @etobella,
some modules you are maintaining are being modified, check this out!

[kobros-tech]

dnplkndll

[flotho]

thanks
tested on runboat,
LGTM

[xaviedoanhduy]

LGTM about functionality and code review

[OCA-git-bot]

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). 🤖

[sergiocorato]

Thanks

[victoralmau]

My other comments are pending: #86 (comment) + #86 (comment) + #86 (comment) + #86 (comment)

[kobros-tech]

My other comments are pending: #86 (comment) + #86 (comment) + #86 (comment) + #86 (comment)
@victoralmau

There are mock tests and tour tests already there, you can review!
If you don't like number of rows I added as 10, what is the default should be there?
The new breadcrup should be now within others smoothly, so please review agin these issues properly.

Any other ones that could be related to new work will be handled according to what we decide whether remving or keeping to resume work.

I expect to stop at this point as the commit is getting large and review will never stop, so I tend to remove the lement for upcomming contribution.

Thanks ;)

could you guide me if I add my tour how can user open a page and return back to open another one like mine.
I think I tried but I got timeout.

[victoralmau]

My other comments are pending: #86 (comment) + #86 (comment) + #86 (comment) + #86 (comment)
@victoralmau

There are mock tests and tour tests already there, you can review! If you don't like number of rows I added as 10, what is the default should be there? The new breadcrup should be now within others smoothly, so please review agin these issues properly.

Any other ones that could be related to new work will be handled according to what we decide whether remving or keeping to resume work.

I expect to stop at this point as the commit is getting large and review will never stop, so I tend to remove the lement for upcomming contribution.

Thanks ;)

Please comment on each thing in the corresponding conversation, if you want to. You can also skip my review comments, of course.

[kobros-tech]

@victoralmau
I asked you if you don't like this default value to show:
self._items_per_page = 10

which is the other default one? as in the method I set the value by the way
I choose how many lines to appear, if you have an idea I didn't catch up explain more and better to have an example with code.

https://github.com/odoo/odoo/blob/18.0/addons/portal/controllers/portal.py#L133
https://github.com/odoo/odoo/blob/18.0/addons/hr_timesheet/controllers/portal.py#L77

For tests we have tour test and methods tests, including mocking if you don't like mock then it is a solution better than none.
tour is good but functionality for each part is good to be tested too.

We do mocking in avatax not just here.

I see the PR is very ready now

[kobros-tech]

@etobella
@victoralmau
@pedrobaeza

You have made entire change on the module by deleting many files and a dir, it is ok to remove vunrable code but now I have conflict and I have to rebuld my work from zero as long as WE want it to be merged into OCA.

So, can we have a serious look into the open PRs and prepare for merging, I am ready for it and waiting.

Then, I can fix the scroll PR with the conflict.

[pedrobaeza]

It's true that #105 removes code, but it reduces a lot of duplicated code, don't you think? I recommend you also to subscribe to the repository to be aware of the current pull requests, for commenting about before merging, so the rest can be aware.

[kobros-tech]

It's true that #105 removes code, but it reduces a lot of duplicated code, don't you think? I recommend you also to subscribe to the repository to be aware of the current pull requests, for commenting about before merging, so the rest can be aware.

all right, I would like to know how to subscribe to a repository in OCA?

[pedrobaeza]

Here you have it:

[kobros-tech]

@pedrobaeza

I really need to have this module merged as long as there is no more issues to solve.

between time to time I find conflicts with the base branch and I have to fix the conflict, we still have a migration to 18.0 of this feature and the PR is closed although it is a concerning feature, right?

#77

[kobros-tech]

@etobella
@pedrobaeza

Are we ready now?

[pedrobaeza]

@etobella you tell us

[etobella]

When I download the file from here:

The file is downloaded without an extension.

[victoralmau]

IMO, I think an attachment_id store=False field should be created with this domain in sign.oca.request.signer to simplify the code https://github.com/OCA/sign/blob/16.0/sign_oca/models/sign_oca_request.py#L289

Furthermore, I believe that the portal_download_signed() method is unnecessary because it could be used for the download /web/content/{attachment_id} url (something that the web allows: https://github.com/odoo/odoo/blob/215ad0e1c7d74f919c646e40ed23c0bb12cee034/addons/web/controllers/binary.py#L65).

[kobros-tech]

@victoralmau

I did not understand what do you mean in the first sentence, can you explain more the difference and the requested change, please?

[kobros-tech]

@victoralmau

return request.redirect(
f"/web/content/{attachment.id}?download=true&filename={utf8_filename}"
)

using this line is very nice but the module was not compatable with it, if I use it I need access_token value for the attachment or for the sign.oca.request.signer or sign.oca.request but sign.oca.request has no field access_token but has attachment

and sign.oca.request.signer has access_token but does not have attachment

[xaviedoanhduy]

using this line is very nice but the module was not compatable with it, if I use it I need access_token value for the attachment or for the sign.oca.request.signer or sign.oca.request but sign.oca.request has no field access_token but has attachment

hi @kobros-tech, you can get access_token from generate_access_token() function from attachment created above
https://github.com/odoo/odoo/blob/e8d4873ecf27d10399b67b62a03cce6c47a2f69b/addons/mass_mailing/models/mailing.py#L1404

[kobros-tech]

@xaviedoanhduy
@victoralmau
@pedrobaeza
@etobella

now the suggestions are added and no issue with access, thanks xaviedoanhduy

[kobros-tech]

hello?
@etobella

[kobros-tech]

I am waiting for celebrating the merge!

[etobella]

/ocabot merge minor

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-86-by-etobella-bump-minor, awaiting test results.

[OCA-git-bot]

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