fix: duplicate role-group-fix #3774
Conversation
| } | ||
| func (impl RoleGroupRepositoryImpl) GetRoleGroupByCasbinName(name string) (*RoleGroup, error) { | ||
| var model RoleGroup | ||
| err := impl.dbConnection.Model(&model).Where("casbin_name = ?", name).Where("active = ?", true).Order("updated_on desc").Select() |
There was a problem hiding this comment.
what is the need of ordering? if there can be multiple rows in output use limit with this or use slice model
pkg/user/RoleGroupService.go
Outdated
| rgName := strings.ToLower(request.Name) | ||
| object := "group:" + strings.ReplaceAll(rgName, " ", "_") | ||
|
|
||
| _, err := impl.roleGroupRepository.GetRoleGroupByCasbinName(object) |
There was a problem hiding this comment.
are we only using this for checking if entry exists or not? if yes then we should use exists clause since this is confusing. Also, add comments
pkg/user/RoleGroupService.go
Outdated
|
|
||
| roleGroup, err := impl.roleGroupRepository.GetRoleGroupByCasbinName(object) | ||
| exists, err := impl.roleGroupRepository.CheckRoleGroupExistByCasbinName(object) | ||
| if err != nil && err != pg.ErrNoRows { |
There was a problem hiding this comment.
do we still need ErrNoRows check?
pkg/user/RoleGroupService.go
Outdated
| return nil, err | ||
| } else if roleGroup != nil { | ||
| } else if exists { | ||
| impl.logger.Errorw("role group already present", "err", err) |
There was a problem hiding this comment.
add roleGroup name for debugging
|
|
||
| func (impl RoleGroupRepositoryImpl) CheckRoleGroupExistByCasbinName(name string) (bool, error) { | ||
| var model *RoleGroup | ||
| return impl.dbConnection.Model(&model).Where("casbin_name = ?", name).Where("active = ?", true).Exists() |
There was a problem hiding this comment.
using pointer to pointer, fix this
|
@Shivam-nagar23 @kartik-579 @vikramdevtron What is the blocker here? Why are we not merging this? |
|
Kudos, SonarCloud Quality Gate passed!
|
Description
Fixes #3772
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
Does this PR introduce a user-facing change?