-
Notifications
You must be signed in to change notification settings - Fork 60
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
Write Test For GetUserDropdownWorkspaces
#1813
Write Test For GetUserDropdownWorkspaces
#1813
Conversation
Hi @elraphty, Please review this PR. |
handlers/workspaces_test.go
Outdated
|
||
dbPerson := db.TestDB.GetPersonByUuid(person2.Uuid) | ||
|
||
handlerUserHasAccess := func(pubKeyFromAuth string, uuid string, role string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MuhammadUmer44 You should not mock this, the HasAccess Status should come from the controller/ business logic, mocking this defeats the purpose of this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elraphty I check it
@MuhammadUmer44 what's the status of the requested change? |
Hi @elraphty, Please review this PR. |
@@ -1394,6 +1394,20 @@ func (db database) GetPerson(id uint) Person { | |||
return m | |||
} | |||
|
|||
func (db database) DeleteWorkSpaceAllData() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put these functions in the db/test_config.go file, since we only need them for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elraphty ok
handlers/workspaces_test.go
Outdated
db.TestDB.DeleteWorkSpaceAllData() | ||
|
||
oHandler := NewWorkspaceHandler(db.TestDB) | ||
oHandler.userHasAccess = db.TestDB.DeleteWorkSpaceUserAccessData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MuhammadUmer44 This defeats the purpose, the function returns true it does not check if the user has access.
{WorkspaceUuid: workspace.Uuid, OwnerPubKey: person2.OwnerPubKey, Role: "ADD BOUNTY"}, | ||
{WorkspaceUuid: workspace.Uuid, OwnerPubKey: person2.OwnerPubKey, Role: "UPDATE BOUNTY"}, | ||
{WorkspaceUuid: workspace.Uuid, OwnerPubKey: person2.OwnerPubKey, Role: "DELETE BOUNTY"}, | ||
{WorkspaceUuid: workspace.Uuid, OwnerPubKey: person2.OwnerPubKey, Role: "PAY BOUNTY"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a "VIEW REPORT" role to the user, which will make the user have 5 roles, it will fix the userHasAccess error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elraphty I have try but i got same error:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped a comment
handlers/workspaces.go
Outdated
bountyCount := db.DB.GetWorkspaceBountyCount(uuid) | ||
hasRole := db.UserHasAccess(pubkey, uuid, db.ViewReport) | ||
bountyCount := oh.db.GetWorkspaceBountyCount(uuid) | ||
hasRole := oh.userHasAccess(pubkey, uuid, db.ViewReport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserHasAccess is in the Database interface so you can change it to oh.db.UserHasAccess
, let me know if this fixes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elraphty The same error still persists. I tried everything, but the UserHasAccess error is not fixed. Without verifying UserHasAccess, the unit test does not pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Push your latest changes, let me give it a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@elraphty |
@AbdulWahab3181 can you look into this? |
@elraphty sure, Let me take a look |
HI @elraphty, |
Describe your changes
Issue ticket number and link
Closes: #1805
Checklist before requesting a review