🇮🇪Ireland @lostcarpark

lostcarpark → created an issue.

Merged into 1.1 and 1.2 branches, moving to fixed.

Went through with @ultimike, and he suggested removing <front> from the access check, as we're no longer using it as a route.

Thanks for the review, @ant1.

Going ahead with a merge.

Merged successfully. A nice improvement to test coverage.

Thanks for working on this.

I've reviewed and run the tests locally, and this change looks perfect to me.

Moving to RTBC.

jdleonard → credited lostcarpark → .

Yes there is "clone issue" in sidebar! Never noticed that before. Maybe when in the final documentation we could have a screenshot.

Added 1.5 about Volunteer app.

@mradcliffe - you forgot the most important step - updating the icebreaker question!

More seriously, I don't see the clone function, and Dreditor project has been abandoned 5 years ago, and I'm not sure there's an install for it still available.

lostcarpark → created an issue. See original summary → .

That looks perfect now, and all tests are passing.

Moving to RTBC.

@lavanyatalwar, thank you for working on this issue, and thanks for your patience with my nit-picking.

Merge completed, moving to fixed.

That looks perfect now, and all tests are passing.

Moving to RTBC.

Sorry to be picky, but I think the description could be a tiny bit clearer.

Also, sorry for not including in the original request, but I think we should have a test case for this. This can be added in VerifyEmailSetupTest.php, in the testVerifyEmailConfig function, just before submitting the form. Add the following assertion:

$this->assertSession()->elementExists('xpath', '//div[@id="edit-email-body--description"][contains(text(), "Email message to be sent to user. Place token [verify-email:url] to insert the login link")]');

I promise this is the last change!

Thanks for working on this. Some change to the text is needed. In particular, it needs to reference the available token.

Adding contributors.

Thank you for working on this. Another little step forward.

lostcarpark → created an issue.

Perfect! Moving to RTBC.

Thanks for working on this.

There's just one minor issue, the token should use a hyphen rather than an underscore.

We should then be ready to merge.

Merged and released version 1.2.0.

PHPStan error is due to Project Browser updates for Package Manager being addressed in 📌 Conflict with Project Browser <2.1.0 and Automatic Updates <4.0.0 Active , so not a concern for this module.

I've added a test of pressing the "Clear storage" menu, and verifying the correct response.

I'm moving this to RTBC.

I've completed the addition of a "Tools->Project Browser->Clear storage" option.

Had some difficulty getting tests to run for Next Minor in GitLab CI, but thanks to @cmlara on Slack, I figured out the correct versions to specify in composer.json.

Still getting a PHPStan error for next minor.

lostcarpark → created an issue.

One more small change. In the NavigationExtraToolsAccessTest test, I had hardcoded the strings the test asserts against, which are used multiple times. I have moved these into constants, which reduces duplication, and makes the code more readable.

Another small update.

I figured out the controller core "parent" menus use, which displays the child items as a list.

Using the same controller for the "Tools", "Clear individual cache", and "Development" menus that if a user ends up on one of those pages, they will see available options in a list (or a message letting them know they don't have access to any menu items).

Although this should not happen in normal circumstances, it means that if the user does land on the Tools menu page, the menu will be displayed consistently with other menu pages.

This also means the individual controllers for those pages are no longer required.

Would be very grateful if anyone has time to test.

Suggested test steps:

1. Install current stable version (1.1.x branch) of Navigation Extra Tools.
2. Check that admin user can see Tools menu on sidebar, with cache clearing options under it.
3. Create user with access to Navigation, but no admin functions, and verify that Tools menu displays, and empty "Flush individual cache" submenu shown, but no other options visible..
4. Install this issue fork, and either uninstall/reinstall, or clear caches.
5. Check that Tools no longer shows for user with only access to Navigation.
6. Grant user access to "Navigation Extra Tools Clear Cache", and verify Tools menu visible (and cache clearing options under it).
7. For bonus points, take away their cache clearing privilege, but then install the Devel module, and give user "Access Devel Information" privilege. Check they can see Tools again, and only option under it is "Development".

Obviously will give contribution credit for testers.

After reviewing with @ultimike, I have made some changes:

• I've made both the main tools menu and the Devel menu call the same access function, and use the route name to select which check gets used.
• I was fetching the whole Admin menu, and then selecting the Tools submenu from within it. I've figured out I can use setRoute in the MenuTreeParameters to select the specific submenu I want. I've also told it to only select one level of the menu.
• I was traversing the whole Tools menu tree to find all the items the user has access to. I've realised that once I find one item the user has access to, I know they can see the Tools menu, so I don't need to look any further, so I can return from checkTreeAccess as soon as an item is found.
• I was recursively checking the whole tree under Tools menu, but I've realised any submenus under Tools will have their own permission, so I only need to check for access to items on the first level below Tools. That allows me to drop the recursion, so makes the check simpler.

Change merged successfully.

Thanks for your work on this!

I have reviewed the test, and it looks perfect to me. It confirms that the correct result is being returned from the token replacement, and the custom token is applied.

The tests are passing again, and I have run locally to confirm.

Moving to RTBC.

Merged. Moving to fixed. Thanks for working this, @nidhi27.

I've reviewed this change and am happy to merge. Hopefully we are moving closer to tests passing.

Moving to RTBC.

Accidentally specified the class name as "TokenHooks" when it should have been "TokensHooks". Thanks for catching this, @nidhi27.

Tests currently failing because of empty test class, but will be fixed soon.

Merged, though we have test failures. Will be fixed by 📌 Add test of token replacement Active .

Thank you for working on this.

This looks great to me.

The tests are producing a warning because there are no tests in the class, which is getting marked as a failure in PHPUnit. However, this should be resolved in the next issue.

Moving to RTBC.

Thank you for your work on this issue. Now merged.

I have reviewed and carried out some testing, and I can confirm this behaves as expected.

Moving to RTBC.

Thanks for your work on this.

Looks perfect. Merging.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

Thanks for your work on this. Moving to fixed.

lostcarpark → created an issue.

I've reviewed and carried out some very basic testing, and I believe this change is ready to merge.

Moving to RTBC.

Merge complete. Now moving to fixed.

Thank you for your work on this issue.

Looks good now.

There is a PHPCS warning, but that is due to an earlier merge and expected.

Moving to RBTC and will merge shortly.

Looks good. I will complete review shortly.

Changed merge target to 1.1.x.

Unfortunately, as 📌 Add token info hook Active modified the same file, this creates a merge conflict. Could you please rebase?

Accidently changed status on wrong issue. Setting back to active.

Changed merge target to 1.1.x.

Unfortunately, as 📌 Add token info hook Active modified the same file, this creates a merge conflict. Could you please rebase?

Thanks for your work on this!

Just a couple of small fixes needed. Please use the class namespace and name for the service name, so it can be referenced by TokenHooks::class. Also, please move the position in services.yml, so that hook services are grouped together.

Thanks for working on this.

Oh wait, apologies, I didn't notice the comment was on the parent issue. Setting back to active.

Thank you, @lavanyatalwar!
Just for future reference, when you complete work on an issue, you should update the status to "needs review". Otherwise, it can be easy for reviewers to overlook it. I'm updating the status for you now, and will review shortly.

This looks good on initial review. I will complete review with TokenInfo hook.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

Sorry for the shenanigans around merge requests. I accidentally merged into 1.0. I have now merged into 1.1, and reverted from 1.0.

Moving to fixed, ready for follow up issues to do the real work.

Thanks for your work on this.

Looks perfect now, and tests are passing (except for PHPCS warning about unused use statement, which is expected).

Moving to RTBC.

Than you for working on this. There is just one small change needed to remove the attribute on the class. The attribute should be on functions within the class, not on the class itself.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

smustgrave → credited lostcarpark → .

nicxvan → credited lostcarpark → .

This is very interesting to me.

As well as "fixed" or "rolling" membership, it would be great to support custom membership terms.

For example, there's an organisation I'm part of where membership was originally yearly, but they produce a magazine that originally came out quarterly, but amateur magazines often struggle to follow strict schedules, so it became less regular, usually every four months or so. To keep things fair, they redefined the member as "four issues of the magazine", so when you join, you can't be sure when your membership will end.

I'm not saying the module should handle this sort of exceptional case, but it should provide a way for organisations to implement it. Using a plugin to define membership term like the membership module as @freelock mentions in #6 sounds like a good approach to take.

Perhaps we could provide "fixed" and "rolling" as two built in plugins, and allow organisation that need more complex rules, or integration with an external source, to implement their own plugin.

jdleonard → credited lostcarpark → .

jdleonard → credited lostcarpark → .

lostcarpark → created an issue.

lostcarpark → created an issue.

Thank you for reporting and submitting the fix, @seanpb. The test coverage explicitly covers this now, so hopefully it won't cause further issues.

Thanks @pameeela, I wouldn't say you were way off, just that it's one of those issues that initially looks straightforward, but gets a lot more involved when you look into the details. I think this solution should set the module up for possible additions to the Tools menu in future.

If you have time to review, I'd be grateful. It would be nice to get wrapped up into a release.

Rebased after merging 🐛 JS error caused by toolbar library when toolbar is not present Active .

I would have liked to have a review, but as this is affecting site performance, I would like to get the fix into a release, I'll have to rely on the test coverage. Moving to RTBC.

I have added handlers for the menus with children to the Controller, and set their permissions accordingly.

Have added test cases to verify correct permissions.

Needs review.