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

circulation: Support self checkout by patrons #1213

Conversation

sakshamarora1
Copy link
Contributor

@sakshamarora1 sakshamarora1 self-assigned this Jun 3, 2024
@sakshamarora1 sakshamarora1 force-pushed the feature/self_checkout_permissions branch from 2c9b2d3 to 674f4ca Compare June 3, 2024 15:13
@@ -315,3 +315,6 @@
},
)
)

# Feature Toggles
CIRCULATION_SELF_CHECKOUT_ENABLED = SELF_CHECKOUT_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify why is this assignment needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since, circulation is an extension it uses the circulation config and not the main config, so this is needed to get the value from the main config.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't fit this variable under circulation because we are not bringing this change to the circulation module. The prefixes like CIRCULATION_ or ILS_ are meant to hint from which module the variable is coming from

@@ -1071,3 +1078,6 @@ def _(x):
ILS_PATRON_SYSTEM_AGENT_CLASS = SystemAgent

DB_VERSIONING_USER_MODEL = None

# Feature Toggles
SELF_CHECKOUT_ENABLED = False
Copy link
Contributor

Choose a reason for hiding this comment

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

to keep the config variables naming consistent:

Suggested change
SELF_CHECKOUT_ENABLED = False
ILS_SELF_CHECKOUT_ENABLED = False

"bulk-loan-extension",
]
is_authenticated_user = current_app.config.get(
"ILS_AUTHENTICATED_USER_PERMISSIONS", []
Copy link
Contributor

Choose a reason for hiding this comment

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

well done for caring for flexibility of the configuration!
In this case it is a bit overkill though, because we already had the config:

ILS_VIEWS_PERMISSIONS_FACTORY = views_permissions_factory

# 1. self checkout feature flag is enabled
# 2. The patron ids match
if (
kwargs.get("self_checkout", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

just a reminder: we agreed to check if the action is performed by the librarian, not to pass the kwarg from the UI, since it is more reliable check.

Comment on lines 199 to 203
if (
kwargs.get("self_checkout", False)
and current_app.config.get("CIRCULATION_SELF_CHECKOUT_ENABLED", False)
and patron_pid != current_user.id
):
Copy link
Contributor

@kpsherva kpsherva Jun 6, 2024

Choose a reason for hiding this comment

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

for readability (check if the logic is correct ;) the point is to have less content heavy conditional statements to be able to understand the bussiness logic behind it easier:

Suggested change
if (
kwargs.get("self_checkout", False)
and current_app.config.get("CIRCULATION_SELF_CHECKOUT_ENABLED", False)
and patron_pid != current_user.id
):
self_checkout_enabled = current_app.config.get("CIRCULATION_SELF_CHECKOUT_ENABLED", False)
is_librarian = .... check if librarian
is_patron_current_user = patron_pid == current_user.id
if self_checkout_enabled and (not is_librarian or not is_patron_current_user):

@sakshamarora1 sakshamarora1 force-pushed the feature/self_checkout_permissions branch from 8ae4a08 to 89635a3 Compare June 7, 2024 08:15
@sakshamarora1 sakshamarora1 force-pushed the feature/self_checkout_permissions branch from 98e9907 to 1eb37bc Compare June 7, 2024 08:28
@kpsherva kpsherva merged commit f9bb9b9 into inveniosoftware:master Jun 7, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self-checkout station
2 participants