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] [mig] pos_user_restriction #649

Merged
merged 12 commits into from
Jun 24, 2021

Conversation

hkapatel-initos
Copy link
Contributor

@hkapatel-initos hkapatel-initos commented May 14, 2021

Migration to 14.0.
In v12.0 there is no restricted group at the opening of session time and in version 14.0 system strictly allows to set a minimal access group of point of sale.
Only if there is a default user in version 14, the user will be set. The point of sale session is open
so select Allowed Employees.
Allowed Employees
Then assign "Authorized Employees" Select the same user(cashier) that is in "Assigned POS Only" Use employee credentials to log in to the PoS session and switch cashier.

@hkapatel-initos hkapatel-initos force-pushed the 14.0-mig-pos_user_restriction branch 3 times, most recently from fff2693 to 5a25402 Compare May 21, 2021 11:20
@hkapatel-initos
Copy link
Contributor Author

@etobella It would be nice if you could review the PR.

@hkapatel-initos hkapatel-initos mentioned this pull request May 23, 2021
16 tasks
Copy link
Contributor

@dsolanki-initos dsolanki-initos left a comment

Choose a reason for hiding this comment

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

Functional review LGTM. Some comments that need to be resolved.

pos_configs = self.pos_config_model.with_user(
self.pos_user_assigned_pos.id
).search([])
# self.assertFalse(pos_configs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you have commented this line?

@@ -7,7 +7,8 @@ access_stock_warehouse_pos_user,stock.warehouse pos_user,stock.model_stock_wareh
access_stock_move_pos_user,stock.move pos_user,stock.model_stock_move,group_assigned_points_of_sale_user,1,1,1,1
access_report_pos_order,report.pos.order,point_of_sale.model_report_pos_order,group_assigned_points_of_sale_user,1,1,1,1
access_account_journal_pos_user,account.journal pos_user,account.model_account_journal,group_assigned_points_of_sale_user,1,0,0,0
access_account_bank_statement,account.bank.statement,account.model_account_bank_statement,group_assigned_points_of_sale_user,1,1,1,0
access_account_payment_method_pos_user,account.payment.method pos_user,account.model_account_payment_method,group_assigned_points_of_sale_user,1,0,0,0
access_account_bank_statement,account.bank.statement,account.model_account_bank_statement,group_assigned_points_of_sale_user,1,1,1,0Assigned users
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please check I think Assigned users added at the end of this line?

@@ -14,29 +15,29 @@
/>
</record>

<data noupdate="1">
<data noupdate="0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I forgot it

assigned_user_ids = fields.Many2many(
"res.users",
string="Assigned users",
help="Restrict some users to only access their assigned points of sale. "
"In order to apply the restriction, the user needs the "
"'User: Assigned POS Only' group",
)
set_pos_user_group = fields.Boolean(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need new field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch the cashier according to the code of the Owl Structure in v14 for the switch cashier (employee) calls the (pos_user_group) method to switch cashier easily and can start a session so I have put a new field and method

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not ok.

Try making the group_pos_user_id field a computed field instead, it should probably work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivantodorovich Thanks for the review suggestions are implemented you can re-review it.

pos_configs = self.pos_config_model.with_user(
self.pos_user_assigned_pos.id
).search([])
# self.assertTrue(pos_configs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here please remove the comment from this line.

@hkapatel-initos hkapatel-initos changed the title [14.0] [mig] pos user restriction [14.0] [mig] pos_user_restriction May 26, 2021
@hkapatel-initos hkapatel-initos force-pushed the 14.0-mig-pos_user_restriction branch 3 times, most recently from 1b51b18 to e437619 Compare May 27, 2021 09:42
@hkapatel-initos
Copy link
Contributor Author

@dsolanki-initos that suggestions are implemented you can re-review it

@hkapatel-initos
Copy link
Contributor Author

hkapatel-initos commented May 31, 2021

@chienandalu can you please add this to the 14.0 milestone and merge this PR?

@hkapatel-initos
Copy link
Contributor Author

@eLBati It would be nice if you could review the PR.

@eLBati
Copy link
Member

eLBati commented Jun 3, 2021

@hkapatel-initos sorry I'm not using v14 at the moment so I am not able to evaluate the correctness of the changes

Copy link
Contributor Author

@hkapatel-initos hkapatel-initos left a comment

Choose a reason for hiding this comment

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

@hkapatel-initos sorry I'm not using v14 at the moment so I am not able to evaluate the correctness of the changes

okay

@@ -14,29 +15,29 @@
/>
</record>

<data noupdate="1">
<data noupdate="0">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I forgot it

@hkapatel-initos
Copy link
Contributor Author

@legalsylvain It would be nice if you could review the PR.

@hkapatel-initos
Copy link
Contributor Author

@ivantodorovich can you please review this PR?

Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Small comments otherwise LGTM

I'm pre-approving

)

@api.depends("assigned_user_ids")
def _compute_set_pos_user_group(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def _compute_set_pos_user_group(self):
def _compute_group_pos_user_id(self):

As per OCA guidelines, use _compute_<field_name> for compute method names

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check this comment as I think it hasn't been taken care of ☺️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivantodorovich that suggestions are implemented can you Re-check it

).search([])
self.assertFalse(pos_configs)

self.pos_config_main._compute_set_pos_user_group()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.pos_config_main._compute_set_pos_user_group()

You can probably remove this line

@hkapatel-initos
Copy link
Contributor Author

@HaraldPanten can u please review this pr ?

@hkapatel-initos
Copy link
Contributor Author

@christophlsa can you please review this pr?

Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Functional review 👍

@OCA-git-bot
Copy link
Contributor

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

@legalsylvain
Copy link
Contributor

@hkapatel-initos can you answer to the @ivantodorovich remarks ?
thanks.

@hkapatel-initos
Copy link
Contributor Author

@hkapatel-initos can you answer to the @ivantodorovich remarks ?
thanks.

Done

@ivantodorovich
Copy link
Contributor

Thanks !! 🎉

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-649-by-ivantodorovich-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c8ade04 into OCA:14.0 Jun 24, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 3c8c30f. 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.

9 participants