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

Feature/name email search list users #184

Draft
wants to merge 35 commits into
base: develop
Choose a base branch
from

Conversation

2piJareem
Copy link
Contributor

@2piJareem 2piJareem commented Apr 6, 2023

Checklist

  • I'm using the correct PHP Version (8.1).
  • I've added comments to any new methods I've created or where else relevant.
  • I've replaced magic method usage on DbService classes with the getInstance() static method.
  • I've written any documentation for new features or where else relevant in the docs repo.

Description

Added Login, Name and Email filter to List Users function within Admin menu.
Filter fields match any part of the target string.
"Name" searches first and last names for a match.

Changelog

  • modules/admin/actions/users.php Added filter functionality
  • modules/admin/templates/users.tpl.php Added filter to template

refs:
issues:

Other Information

Docs pull request:

@adam-buckley adam-buckley changed the base branch from master to develop May 18, 2023 04:47
dist/
package-lock.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this and add package-lock.json back in

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 didn't see this earlier. when I remove the dist/ folder from .gitignore I get 8 files form the dist folder and below that want to be committed - Do we want to commit these files as well?
Screenshot 2023-07-06 at 3 17 12 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

Dist should still be left out, we just need to not add packaage-lock.json to .gitignore

.gitignore Outdated Show resolved Hide resolved
system/modules/insights/actions/index.php Outdated Show resolved Hide resolved
@@ -48,3 +56,20 @@ function index_ALL(Web $w)
//send the table to the template using ctx
$w->ctx('insightTable', Html::table($table, 'insight_table', 'tablesorter', $tableHeaders));
}

// Function to recursively check if a user is a member of a group (or parent group)
function checkUserAccess(Web $w, $group, $user_id) : bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid putting extra functions in action files. They should instead be in the Service class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After putting the function into InsightService
eg public function checkUserAccess(Web $w, $group, $user_id): bool

How does the action function find the function in the service class?
I get "undefined function" if called as "checkUserAccess(...)"

and "Non static method 'checkUserAccess' should not be called statically." if I called as "InsightService::checkUserAccess(...)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw should "checkUserAccess(...)" function go in InsightService or AuthService?

dist/
package-lock.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Dist should still be left out, we just need to not add packaage-lock.json to .gitignore

system/templates/base/dist/app.css Outdated Show resolved Hide resolved
@chris-bateman chris-bateman mentioned this pull request Feb 20, 2024
32 tasks
@adam-buckley adam-buckley marked this pull request as draft March 18, 2024 05:30
@adam-buckley
Copy link
Contributor

Moved to draft cause I dont think this PR offers anything anymore. Holding off to make sure soon

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.

4 participants