Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The Commerce limit Subscriptions module restrict users to buy subscription again if they have already an active subscriptions.
Comments
Comment #2
vishal.kadamThank you for applying! Reviewers will review the project files, describing what needs to be changed.
Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.
To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.
While this application is open, only the user who opened the application can make commits to the project used for the application.
Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.
Comment #3
vishal.kadam@kishan@lnwebworks Remember to change the status to Needs review when the project is ready for review.
Comment #4
kishan@kk CreditAttribution: kishan@kk commentedNeeds Review
Comment #5
vishal.kadamRemove the README.txt file since the README.md file is already present.
Comment #6
kishan@kk CreditAttribution: kishan@kk commentedRemoved the README.txt file
Comment #7
vishal.kadamRest looks fine to me.
Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.
Comment #8
apadernosrc/EventSubscriber/LimitSubscriptions.php
The short description does not explain what its purpose is. For a
LimitSubscriptions
class Provides the default LimitSubscriptions. does not make much sense.There are two short description. Neither of them uses the class namespace.
The last line is extraneous.
The documentation comment does not describe the method parameters.
$this->messenger->addError("You have already added subscriptions product in cart");
Messages passed to the messenger needs to be translatable strings.
subscriptions product is probably supposed to be product subscriptions.
There are also many empty lines that need to be removed.
/commerce_limit_subscriptions.module
$output .= '<p>' . t('The Commerce limit Subscriptions module restrict user to buy subscription again if he have already active subscriptions...') . '</p>';
It should be restricts users. (module is singular, not plural.)
Comment #9
kishan@kk CreditAttribution: kishan@kk commentedThank you for the feedback.
1. Added short description for explanations.
2. Added class namespace in short description.
3. Removed the extraneous line
4. Removed the empty spaces.
5. Described the method parameters in documentation
6. Translatable strings added
7. Updated restricts users
Above changes are made in the default branch.
Comment #10
alvar0hurtad0The info.yml file maybe needs some improvemets:
Please check how this works:
https://www.drupal.org/docs/develop/creating-modules/let-drupal-know-abo...
Comment #11
kishan@kk CreditAttribution: kishan@kk commentedThank you for the feedback
dependencies:
- commerce:commerce
- commerce_recurring:commerce_recurring
Make the above changes in info.yml file
Comment #12
kishan@kk CreditAttribution: kishan@kk commentedComment #13
alvar0hurtad0Some code enhancements.
in line 154:
if ($has_field && !empty($has_field)) {
the
$has_field &&
part is redundant as it's checked in the second condition.Same in line 161:
if ($quantity >= 2 && $has_field && !empty($has_field)) {
and 178:
if ($subscription_products_count > 1 && $has_field && !empty($has_field)) {
In line 185:
$this->cartManager->removeOrderItem($order, $order_item);
$order_item may be NULL if
$cart->getItems()
is null, so WSOD could happen.In line 199:
$this->messenger->addError($this->t('You have already active subscriptions'));
The message should end with a
.
Comment #14
kishan@kk CreditAttribution: kishan@kk commentedThank you for the feedback @alvar0hurtad0
1.
if ($has_field && !empty($has_field))
the $has_field && part is redundant as it's checked in the second condition.
Similar issue are fixed.
2.
$order_item may be NULL if $cart->getItems()
is null, so WSOD could happen.Not empty if condition is added for the above.
3.
$this->messenger->addError($this->t('You have already active subscriptions'));
The message should end with a .
. is added in the message.
Above issues are fixed .
Comment #15
alvar0hurtad0Thanks for the fixes @kishan@lnwebworks I still see some enhancements:
line 81:
There's a missing
\
line 193:
if (!empty($query) && !empty($field)) {
$field variable should not be empty but I can't find the initialisation of $field variable in the method.
Comment #16
alvar0hurtad0sorry I forgot to change the status
Comment #17
kishan@kk CreditAttribution: kishan@kk commentedThank you for the feedback @alvar0hurtad0
1.Updated
2.Updated
$field
variable valueComment #18
alvar0hurtad0Hello @kishan@lnwebworks,
sorry for the ping-pong but I still see some enhancements:
Maybe this should be @var \Drupal\Core\Session\AccountProxyInterface as the rest of attributes.
Comment #19
kishan@kk CreditAttribution: kishan@kk commentedThank you for the feedback @alvar0hurtad0
Updates this as
@var \Drupal\Core\Session\AccountProxyInterface
Comment #20
vishal.kadamComment #21
apadernoThere is no need to add for LimitSubscriptions class since that is already implicit in Constructs a new LimitSubscriptions object which means Constructs a new instance of the LimitSubscriptions class.
I am not sure an event subscribed is supposed to redirect, since that would avoid other event subscribers can react to the same event; it would also interrupt the module that created that event.
Comment #22
apadernoThank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:
You can find more contributors chatting on the Slack #contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
I thank all the reviewers.