Group Membership Request module extends Group module. It allows users to request a membership to a group and managers (administrators) to approve these requests.

I have worked on this module since the patch in Group module issue queue (#2752603: Request Membership feature). Then it was decided to move it to contributed module.
Since then I reached the stable version, added tests and add new features mentioned bellow

The detailed documentation how to install and configure module in README.MD file

  1. This module provides GroupEnabler Plugin, which allows to use group module feature to add new member to the group with the specific role.
  2. This module provides a view to control new pending requests.
  3. This modules provides views plugin entity link to perform approve / reject action on GroupContent entities
  4. This module provides necessary group permissions to control all main features of the module.
  5. This module provides integration with state machine module in order to have workflow transitions for status field: new to pending, pending to approved or pending to rejected. All this transitions fie certain events, which can be use, for example to send notifications to members or admins of a group.
  6. This modules provides manager to control membership request programmatically, if we want to develop some custom logic, but it is the same time get advantage of events system

Project link

https://www.drupal.org/project/grequest

Git instructions

git clone --branch '1.0.x' https://git.drupalcode.org/project/grequest.git

PAReview checklist

http://pareview.net/r/107

Comments

LOBsTerr created an issue. See original summary.

kuldeep_mehra27’s picture

kuldeep_mehra27 commented
Status: Needs review » Needs work

Kindly fix below errors
http://pareview.net/r/107

lobsterr’s picture

lobsterr commented
Issue summary: View changes
Status: Needs work » Needs review

@kuldeep_mehra27, I have fixed all issue except for git. I don't know how to pint http://pareview.net/r/107 to specific branch, what I have right now for release.

In my case this branch https://git.drupalcode.org/project/grequest/-/tree/1.0.0

lobsterr’s picture

lobsterr commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: [D8], [D9] Group Membership Request » [D9] Group Membership Request
Issue summary: View changes
jayelless’s picture

jayelless commented

Preliminary

You have given the git clone url as https://www.drupal.org/project/grequest when it should be https://www.drupalcode.org/project/grequest. The system does an automatic redirect, but this should be corrected.

You have specified the branch to use as "1.0.0", which would normally be a release tag, and will conflict with a tag of the same name when you get a full release. The branch name should be "1.0.x", which I note exists, and is currently one commit ahead of the branch "1.0.0". I have used branch "1.0.x" for this review. Please update the issue to specify branch "1.0.x" and remove branch "1.0.0" from the repository.

Automated Review

No issues reported.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch. However, note comments above in respect of the branch name(s).
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. + Classes GroupMembershipApproveForm, GroupMembershipRejectForm, and GroupMembershipRequestForm: The constructor function has required parameters following optional ones. All required parameters should come before optional ones.
  2. Test classes GroupMembershipRequestFormTest and GroupMembershipRequestTest use the deprecated methods "assert", "assertEqual" and "assertNotEqual". These will be removed in Drupal 10.0.0.
  3. Classes GroupMembershipApproveForm, GroupMembershipRejectForm, GroupMembershipRequestForm, and GroupMembershipRequestController: the @var declaration for the property $membershipRequestManager requires a fully qualified name.
  4. Classes GroupMembershipApproveForm and GroupMembershipRejectForm: the @var declaration for the property $logger requires a fully qualified name.
  5. Classes MembershipRequestManager, GroupMembershipRequestController and GroupMembershipRequestForm: the @param descriptors for the constructor function require fully qualified names.
  6. Class MembershipRequestManager: All method @param and @return descriptors require fully qualified names.
  7. Test classes GroupMembershipRequestTest and GroupMembershipRequestFormTest: the @var declaration for the property $membershipRequestManager requires a fully qualified name.
  8. Test classes GroupMembershipRequestFormtest and GroupMembershipTest: the called methods "assertNull()", "assertFalse", and "expectExceptionMessage()" appear to be undefined.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

jayelless’s picture

jayelless commented
Status: Needs review » Needs work
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes

Yes, 1.0.0 is a wrong branch name; it's a correct tag name, though. The default branch needs to be set to 1.0.x; after that, the 1.0.0 branch can be removed.

lobsterr’s picture

lobsterr commented
Status: Needs work » Needs review

@jlscott Thank you for a great review. I have fixed all issue except the last one.

Test classes GroupMembershipRequestFormtest and GroupMembershipTest: the called methods "assertNull()", "assertFalse", and "expectExceptionMessage()" appear to be undefined.

These method are defined in "\Drupal\simpletest\TestBase" and "\PHPUnit\Framework\TestCase".

P.S. I am sorry for confusion with tags and branch. I was also confused, what should I provide dev branch or a specific tag.

jayelless’s picture

jayelless commented
Status: Needs review » Needs work

@LOBsTerr: Thanks for fixing the identified issues. I think there are still a few things to fix.

  1. The branch 1.0.0 is still present in the repository. You need to delete this from the remote as well as from your local. Refer to git documentation for instructions on how to achieve this.
  2. In class GroupMembershipRequestValidator, the type-hint for the $constraint parameter to the method "validate" specifies an object of class GroupRequestMembership, whereas it should be class GroupMembershipRequest.
  3. In class GroupMembershipRequestController, the return type in PHPDoc does not match the actual return type for method requestMembershipTitle.
  4. In class MembershipRequestManager the return type in PHPDoc does not match the actual return type for methods "approve" and "reject".
  5. File README.md has a typer in the word "Mainteners"

There are still a few instances where the type-hinting for class properties and method parameters requires a fully qualified name, but the leading "/" is missing (eg GroupMembershipRequestTest property membershipRequestManager. It would be good to fix these at some point.

One other small point I note is that the test method "testUserAccessibility" in class GroupMembershipRequestTest is identical to the method "testGroupMembershipDeletion", so perhaps you have copied the code to generate a new test, but have not yet made the second test specific to the use case.

lobsterr’s picture

lobsterr commented

@jlscott Thank you, for another round of checks. I have removed the branch and fixed all coding issues you have found. I also double checked all fully qualified names.

P.S. I wonder which tools normally are used for such checks. So, I will fix the issues in advance. Or it is completely manual job ?

lobsterr’s picture

lobsterr commented
Status: Needs work » Needs review
jayelless’s picture

jayelless commented
Status: Needs review » Needs work

Thanks @LOBsTerr.

The only issue I now find is that the 1.0.0 branch is still present in the remote repository. You should be able to delete this remote branch with the command "git push origin --delete 1.0.0" (assuming your remote is called "origin"). Read about deleting remote branches here: https://www.educative.io/edpresso/how-to-delete-remote-branches-in-git

I use PhpStorm for my development and testing environment. It has the ability to highlight all significant PHP errors and parameter mis-matches, and has been configured with a number of testing tools and code inspections. This makes finding issues with code much quicker than manually scanning the files.

lobsterr’s picture

lobsterr commented
Status: Needs work » Needs review

I don't know maybe I am wrong, but there is not 1.0.0 branch in the remote repo. I have definitely removed it :(

https://git.drupalcode.org/project/grequest/-/branches

git push origin --delete 1.0.0
error: unable to delete '1.0.0': remote ref does not exist
error: failed to push some refs to 'git@git.drupal.org:project/grequest.git'
jayelless’s picture

jayelless commented
Status: Needs review » Reviewed & tested by the community

@LOBsTerr. You are correct, and I apologise. It must have been a caching error, as the branch continued to show in my copy. I deleted the module folder completely from my environment, and re-cloned it. The branch no longer shows :)

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank 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 IRC #drupal-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 dedicated reviewers as well.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.