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.

Sign up for GitHub

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

Jump to bottom

RDISCROWD-7409 Locks API #933

Merged
merged 13 commits into from
Jun 20, 2024
Merged

RDISCROWD-7409 Locks API #933

merged 13 commits into from
Jun 20, 2024

Conversation

kbecker42
Copy link

@kbecker42 kbecker42 commented Jun 17, 2024 •
edited
Loading

  • Added /api/locks/{project_id}
  • Added unit test.

Example

Request

HTTP GET /api/locks/1

Response

{
	"id": 1,
	"short_name": "hello",
	"created": "2024-06-17T20:33:11.563072",
	"locks": [
		{
			"user_id": "1",
			"task_id": "1",
			"seconds_remaining": 3597.541233062744,
			"name": "user",
			"fullname": "user",
			"email": "user@user.com",
			"admin": true,
			"subadmin": false
		}
	]
}

@coveralls
Copy link

coveralls commented Jun 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9555263096

Details

  • 35 of 40 (87.5%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 93.936%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pybossa/api/project_locks.py 33 38 86.84%
Totals Coverage Status
Change from base Build 9553926579: -0.01%
Covered Lines: 17178
Relevant Lines: 18287
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9566672131

Details

  • 37 of 40 (92.5%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 93.947%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pybossa/api/project_locks.py 35 38 92.11%
Totals Coverage Status
Change from base Build 9553926579: -0.003%
Covered Lines: 17180
Relevant Lines: 18287
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9566671604

Details

  • 37 of 40 (92.5%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 93.947%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pybossa/api/project_locks.py 35 38 92.11%
Totals Coverage Status
Change from base Build 9553926579: -0.003%
Covered Lines: 17180
Relevant Lines: 18287
💛 - Coveralls

@kbecker42 kbecker42 requested a review from peterkle June 18, 2024 17:19
@coveralls
Copy link

coveralls commented Jun 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9569244978

Details

  • 40 of 40 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.963%
Totals Coverage Status
Change from base Build 9553926579: 0.01%
Covered Lines: 17183
Relevant Lines: 18287
💛 - Coveralls

peterkle
peterkle reviewed Jun 18, 2024
View reviewed changes
Copy link

@peterkle peterkle left a comment

Choose a reason for hiding this comment

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

I like the thorough testing and have some comments and questions for you to consider.

test/test_api/test_project_locks.py Outdated
from test.test_api import TestAPI


class TestProjectAPI(TestAPI):
Copy link

Choose a reason for hiding this comment

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

Should this be TestProjectLocksAPI?

test/test_api/test_project_locks.py Outdated
@with_context
def test_project_locks_user_worker(self):
""" Test API should return 401 if user is worker"""
admin = UserFactory.create(admin=True)
Copy link

Choose a reason for hiding this comment

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

admin is not used in this test...is it necessary for another purpose?

test/test_api/test_project_locks.py Outdated
@with_context
def test_project_locks_user_subadmin(self):
""" Test API should work if user is subadmin"""
admin = UserFactory.create(admin=True)
Copy link

Choose a reason for hiding this comment

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

like above coment, admin is also not used

test/test_api/test_project_locks.py Show resolved Hide resolved
test/test_api/test_project_locks.py Outdated

@with_context
def test_project_locks_param_does_not_exist(self):
""" Test API project query when search value does not match"""
Copy link

Choose a reason for hiding this comment

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

Should this description be Test API project query when search param does not match?

pybossa/api/project_locks.py Outdated

def _select_attributes(self, data):
# Get the project.
project, owner, ps = project_by_shortname(data.get('short_name'))
Copy link

Choose a reason for hiding this comment

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

ps is unused.

pybossa/api/project_locks.py Outdated
return []
if (not current_user.is_authenticated or
(not current_user.admin and not current_user.subadmin)):
raise Unauthorized("User not authorized for request")
Copy link

Choose a reason for hiding this comment

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

Are we okay with subadmins accessing project lock data for all projects? If not, we need revise the implementation and that test case.

Copy link
Author

Choose a reason for hiding this comment

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

Updated logic to verify subadmins are included as owner in project here.

@kbecker42
Copy link
Author

Fixed code review comments in 86953d1.

@coveralls
Copy link

coveralls commented Jun 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9571558153

Details

  • 41 of 42 (97.62%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 93.958%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pybossa/api/project_locks.py 39 40 97.5%
Totals Coverage Status
Change from base Build 9553926579: 0.008%
Covered Lines: 17184
Relevant Lines: 18289
💛 - Coveralls

peterkle
peterkle approved these changes Jun 20, 2024
View reviewed changes
Copy link

@peterkle peterkle left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

@kbecker42 kbecker42 merged commit e1d68de into main Jun 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterkle peterkle peterkle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@kbecker42 @coveralls @peterkle