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.

Sign up for GitHub

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

Jump to bottom

[18.0][IMP] sign_oca: enable signing from portal #78

Open
wants to merge 1 commit into
base: 18.0
Choose a base branch
from
Open

[18.0][IMP] sign_oca: enable signing from portal #78

wants to merge 1 commit into from

Conversation

kobros-tech
Copy link

adding a new feature by enabling portal users from signing their documents from portal in addition to signing it from emails too.

@OCA-git-bot
Copy link
Contributor

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

@kobros-tech
Copy link
Author

@pedrobaeza
@victoralmau

here is what you asked me for :)

@pedrobaeza pedrobaeza added this to the 18.0 milestone Jan 24, 2025
@kobros-tech
Copy link
Author

Hi @victoralmau, I am happy with your review here, forgive me for being stressed last week as in order to upgrade some modules I was fixing test cases and some code which was not easy to accomplish.

Best,

victoralmau
victoralmau suggested changes Jan 28, 2025
View reviewed changes
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.

Thank you for adding it to sign_oca directly. I guess #77 could be closed (to avoid confusion).

sign_oca/__manifest__.py Outdated Show resolved Hide resolved
sign_oca/views/sign_portal_oca_templates.xml Outdated Show resolved Hide resolved
sign_oca/views/sign_portal_oca_templates.xml Outdated Show resolved Hide resolved
sign_oca/controllers/portal.py Outdated Show resolved Hide resolved
victoralmau
victoralmau approved these changes Jan 29, 2025
View reviewed changes
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.

A functional comment, if the text indicated is “View, or sign documents” I think that all the requests should be shown (not only the ones pending to sign), what do you think?

@kobros-tech
Copy link
Author

Code and functional review OK.

A functional comment, if the text indicated is “View, or sign documents” I think that all the requests should be shown (not only the ones pending to sign), what do you think?

I will discuss this with Mr. Don.

@kobros-tech
Copy link
Author

@etobella
can we get this merged?

etobella
etobella reviewed Feb 3, 2025
View reviewed changes
sign_oca/controllers/main.py
@@ -109,3 +110,62 @@ def get_sign_oca_sign_access(
return signer_sudo.action_sign(
items, access_token=access_token, latitude=latitude, longitude=longitude
)

def get_sign_requests_domain(self, request):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some tests?

Here you can find some examples:

https://github.com/OCA/sign/blob/16.0/sign_oca/tests/test_sign_portal.py#L49

Copy link
Author

Choose a reason for hiding this comment

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

@etobella
Once I get some time I will add test cases, thanks!

@victoralmau victoralmau mentioned this pull request Feb 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etobella etobella etobella left review comments

@victoralmau victoralmau victoralmau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
18.0
Development

Successfully merging this pull request may close these issues.
5 participants
@kobros-tech @OCA-git-bot @victoralmau @etobella @pedrobaeza