-
-
Notifications
You must be signed in to change notification settings - Fork 376
[18.0][MIG] document_page_approval: Migration to 18.0 #524
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
[18.0][MIG] document_page_approval: Migration to 18.0 #524
Conversation
|
Can you please rebase on latest 18.0 - that should fix the tests. |
(../7.0pageapproval/ rev 1) fix status bar in page approval (../7.0pageapproval/ rev 2) change approval workflow in page history and started to add showing approved page in normal page (../7.0pageapproval/ rev 3) show last approved content and aproved by (../7.0pageapproval/ rev 4) Fix bug on page creation (../7.0pageapproval/ rev 5) add status in history tree view (../7.0pageapproval/ rev 6) add a need approval field and hide the workflow when no approval needed (../7.0pageapproval/ rev 7) add access validation to approval (../7.0pageapproval/ rev 8) give rigth to Page aprover group on the page_history model and hide the button for unallowed group thought the security isn't enforced a the orm level (../7.0pageapproval/ rev 9) made invisible the prover group when not needed (../7.0pageapproval/ rev 10) add email to aprovers group's members (../7.0pageapproval/ rev 11)
[ADD] Translation file + french [FIX] employee_id field does not exist [FIX] user_email field deprecated [IMP] Split long lines [ADD] Images
fix act_draft and minor ui fix remove unused variables change order of xml data files
It should work but further errors could help understand what's going Commented other necessary part Add back workflow Add dependency to knowledge
[IMP] - Improved views to match document_page improvements. [IMP] - Added notification to followers when a new version is approved. [ADD] - Updated document_page_approval pot and added es translations. [IMP] - document_page_approval: Updated spanish translation.
…te module is installed.
* Lint error corrected * test improved * lint error corrected in test file * readme corrected * test improved * test of document page history workflow added * CI error corrected * dreipst comment * Migration to new api improved in model's file and data tag remove in xml file * code totally migrate to odoo 9.0 api * test bug fixed * test error fixed * Lint error improved
…ovements on documents (OCA#155)
* Rename classes and files to match model names, and removed workflow engine. * Some fields should be readonly if not in draft. * Allow Knowledge / Manager to edit previous history names and summary * Improve ux by adding user_has_drafts to document pages. * Changes because of implementation of active field on document_page * Add ir.rules for document.page.history * Allow to quickly cancel&draft from 'to approve' state * FIX BUG: am_i_approver was being run as sudo(), hence always giving the user Approving rights. * FIX hooks Because of default='draft' on the state field, state IS NULL wasn't selecting anything. Left the condition in case this is changed in the future.
[ADD] icon.png
Add the the group eployee to the rule "Change Request Global" to allow to the user with the rule "Change Request Approver" to create pages
[UPD] Update document_page_approval.pot
To pass test is necesary set approval required to False in demo data
[UPD] Update document_page_approval.pot [UPD] README.rst
TT36950 [UPD] Update document_page_approval.pot [UPD] README.rst
[UPD] Update document_page_approval.pot [UPD] README.rst Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/
Currently translated at 100.0% (61 of 61 strings) Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/it/ Translated using Weblate (Italian) Currently translated at 100.0% (61 of 61 strings) Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/it/
Currently translated at 83.6% (51 of 61 strings) Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/hr/ Translated using Weblate (Croatian) Currently translated at 96.7% (59 of 61 strings) Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/hr/
Currently translated at 100.0% (61 of 61 strings) Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/it/
Currently translated at 100.0% (61 of 61 strings) Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/it/
[UPD] Update document_page_approval.pot document_page_approval 16.0.1.1.0 Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: knowledge-16.0/knowledge-16.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-16-0/knowledge-16-0-document_page_approval/
Currently translated at 98.3% (61 of 62 strings) Translation: knowledge-17.0/knowledge-17.0-document_page_approval Translate-URL: https://translation.odoo-community.org/projects/knowledge-17-0/knowledge-17-0-document_page_approval/es/
72f8257 to
feb3997
Compare
|
@dreispt, Please Review. It's done. |
| "{}/web#db={}&id={}&" "model=document.page.history" | ||
| ).format(base_url, self.env.cr.dbname, page.id) | ||
| f"{base_url}/web#db={self.env.cr.dbname}&id={page.id}&" | ||
| "model=document.page.history" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nota that these URLs are still supported, but have changed and might stop working in future versions.
|
/ocabot migration document_page_approval |
| cleaned_diff = re.sub(r'data-last-history-steps=".*?"', "", raw_diff) | ||
|
|
||
| rec.diff = cleaned_diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this? Sorry, but I don't understand (if it is necessary, perhaps it would be appropriate to explain in the method the reasons, so that we all know it in the future).
| # Ensure test-user2 has write access on document.page.history | ||
| cls.env["ir.model.access"].create( | ||
| { | ||
| "name": "Write Access for Test User2", | ||
| "model_id": cls.env["ir.model"]._get_id("document.page.history"), | ||
| "group_id": cls.approver_gid.id, # Assign to approver group | ||
| "perm_read": True, | ||
| "perm_write": True, | ||
| "perm_create": True, | ||
| "perm_unlink": True, | ||
| } | ||
| ) | ||
|
|
||
| # Disable restrictive record rules for the test | ||
| cls.env["ir.rule"].search( | ||
| [("model_id", "=", cls.env["ir.model"]._get_id("document.page.history"))] | ||
| ).write({"active": False}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why these changes are necessary. In migrations, the proper thing to do is to reduce the diff (for better readability).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have to add this because of apply write access to document.page.history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, doing this in a test is not necessary for at least 99%, and in this case it is not. Doing this indicates that something defined in the test is incorrect. Checking in detail I have already detected what happens, I suggest it now (remember, a test should test a real case, and there was some unnoticed but important step that was not being done).
| cls.user2 = new_test_user(cls.env, login="test-user2", groups="base.group_user") | ||
|
|
||
| # Ensure user2 has the approver group | ||
| cls.approver_gid = cls.env.ref( | ||
| "document_page_approval.group_document_approver_user" | ||
| ) | ||
| cls.user2.write({"groups_id": [Command.link(cls.approver_gid.id)]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cls.user2 = new_test_user(cls.env, login="test-user2", groups="base.group_user") | |
| # Ensure user2 has the approver group | |
| cls.approver_gid = cls.env.ref( | |
| "document_page_approval.group_document_approver_user" | |
| ) | |
| cls.user2.write({"groups_id": [Command.link(cls.approver_gid.id)]}) | |
| cls.user2 = new_test_user(cls.env, login="test-user2", groups="document_page_approval.group_document_approver_user") | |
| cls.approver_gid = cls.env.ref( | |
| "document_page_approval.group_document_approver_user" | |
| ) |
The base.group_user permission is inherited indirectly with document_knowledge
feb3997 to
d9e5bdd
Compare
|
@victoralmau, Please Review.it's done. |
victoralmau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you can remove the code from #524 (comment)
| self.assertNotEqual(page.approved_date, chreq.approved_date) | ||
| self.assertNotEqual(page.approved_uid, chreq.approved_uid) | ||
|
|
||
| chreq.action_to_approve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to remove it, it is important.
|
|
||
| chreq.action_to_approve() | ||
| # Move change request to 'to approve' state | ||
| chreq.with_user(self.user2).action_to_approve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary, it is already in “to approve” status requested by the user who created the history.
|
|
||
| chreq.action_approve() | ||
| # Approve the change request | ||
| chreq.with_user(self.user2).action_approve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want user2 to approve it, you need it to be in “to approve” status, so you will need to do the following first
chreq.action_to_approve()
self.assertEqual(chreq.state, "to approve")
|
Please, cherry-pick #531 to commit history before migration commit. |
d9e5bdd to
262f507
Compare
|
@victoralmau, Please Review.i updated test cases. |
Steps to reproduce the error: - Modify the content of a page (with a different user). - Go to the Change request menu and Cancel the change. - Click on the "Back to draft" button. - An error will be displayed TT55569
262f507 to
a8346c2
Compare
victoralmau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, good job.
Remember for the future that it is not necessary to add line breaks. I comment this because it helps to reduce the diff of the migration commit and is more readable.
|
This PR has the |
|
/ocabot merge nobump |
|
This PR looks fantastic, let's merge it! |
|
Congratulations, your PR was merged at 811b070. Thanks a lot for contributing to OCA. ❤️ |
No description provided.