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

API Convert SecurityAdmin to be a ModelAdmin #963

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville commented Oct 4, 2019

This is a bit experimental. It converts SecurityAdmin to be a ModelAdmin.

It seems to work remarkably well. If we think this is worthwhile, we can take it forward and finalise it.

Couple things to address:

  • Rename Members to Users ... that is if we prefer that label.
  • Bring back the "Caution: Removing members from this list will remove them from all groups and the database"
  • I think there's a bit of custom import logic that I didn't copy over. I need to dig deeper and see if differs from the boiler plate ModelAdmin import logic.
  • Clean up any redundant JS and SS files

Parent issue

Copy link
Contributor

@bergice bergice left a comment

Choose a reason for hiding this comment

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

I agree with renaming from Users to Members.

The warning message is no longer displayed and needs to be re-added:
image

Some reasons why we should consider leaving the changes the way they are:

  • The tabs now load when opened, making for a slower user experience.
  • We will likely introduce a couple of bugs with these changes, requiring further work
  • What are we gaining with making this change?

@maxime-rainville
Copy link
Contributor Author

I agree that it's not definitely clear that this is something worth doing and that there is some risk of regression. The big plus I see for doing this are that it reduces/simplifies the code base we have to maintain in the long run and any future bug fix we apply to ModelAdmin will automatically be applied to SecurityAdmin.

@maxime-rainville
Copy link
Contributor Author

maxime-rainville commented May 1, 2020

Might be worth waiting until we have some sizeable chunk of work to do to SecurityAdmin.

@emteknetnz
Copy link
Member

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

@maxime-rainville
Copy link
Contributor Author

FYI I had a look at applying the Versioned extension to Member. The resulting Security section wasn't working great ... presumably because Security Admin is not a ModelAdmin.

silverstripe/silverstripe-framework#9671

@emteknetnz
Copy link
Member

I'm not sure what that means in relation to this pull request? Is this pull-request still useful and you'll do further work on this or is it something we should now close?

@maxime-rainville
Copy link
Contributor Author

I think the pull request is still useful. Maybe I should have created a parent issue with the overarching context.

The main problem is that SecurityAdmin reimplement all the features of ModelAdmin without being a ModelAdmin. This increase the amount of code we have to maintain and increase the risk that we'll introduce new bugs because we have two classes to maintain that do exactly the same thing.

@maxime-rainville
Copy link
Contributor Author

Current assumption is that there's nothing controversial about this. It would be a good way to reduce technical debt.

@emteknetnz
Copy link
Member

Closed in favor of #1359

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.

3 participants