SURFnet / rd-sram-integration

Research Drive / SURF Research Access Management Integration
2 stars 3 forks source link

refactor getGroup/getGroups #215

Closed soltanireza65 closed 1 year ago

soltanireza65 commented 1 year ago

instead of calling groupManager->get($gid); now we are instantiating a new Group object directly in handleUpdateGroup

    private function getGroupObject(string $groupId): Group {
        return new Group($groupId, [$this->groupBackend], $this->userManager, $this->eventDispatcher, $this->groupManager, $groupId);
    }
private function handleUpdateGroup(string $groupId, $obj) {
        $group = $this->getGroupObject($groupId);
        ...
}

plus some refactoring to cleanup getGroup/s

issue link: https://github.com/SURFnet/rd-sram-integration/issues/197

thepeak99 commented 1 year ago

Hi! why are you creating a new object every time? Maybe there was a discussion that I missed :cry:

soltanireza65 commented 1 year ago

@thepeak99 can you take a look at the issue? https://github.com/SURFnet/rd-sram-integration/issues/197 we talked there and agreed on this the short answer (just because of the master/slave database situation)

thepeak99 commented 1 year ago

Wait I thought we agreed to put a small cache in the GroupBackend class, at the end you ended up wrapping the GroupManager with the famous proxy object :D

I mean it's okay too, I just want to know why didn't you just put it in the GroupBackend as we discussed instead of creating a proxy object.

soltanireza65 commented 1 year ago

I just started to do as we agreed, but then I just realized that the manager is listening to the postDelete event and modifying cachedGroups and also I just decided not to use GroupBackend directly inside the controller (controller should not directly deal with DB)

thepeak99 commented 1 year ago

Perfecto! So could you please delete the commented code (or comment why it should stay there :D) and we can merge :grimacing: removed,