PR#842: frontend: allow admin to edit package - copr/copr

copr / copr

#842 frontend: allow admin to edit package

Merged 4 years ago by thrnciar. Opened 4 years ago by thrnciar.

copr/ thrnciar/copr edit-package-conf-as-admin into master

frontend: allow admin to edit package

Tomas Hrnciar • 4 years ago

8e8867b

file modified

+5 -5

		`@@ -115,21 +115,21 @@`
		`"""`
		`Determine if this user can build in the given copr.`
		`"""`
		`- can_build = False`
		`+ if copr.user.admin:`
		`+ return True`
		`if copr.user_id == self.id:`
		`- can_build = True`
		`+ return True`
		`if (self.permissions_for_copr(copr) and`
		`self.permissions_for_copr(copr).copr_builder ==`
		`helpers.PermissionEnum("approved")):`
		`-`
		`- can_build = True`
		`+ return True`

		`# a bit dirty code, here we access flask.session object`
		`if copr.group is not None and \`
		`copr.group.fas_name in self.user_teams:`
		`return True`

		`- return can_build`
		`+ return False`

		`@property`
		`def user_teams(self):`

frontend/coprs_frontend/tests/test_api/test_build_r.py

file modified

		`@@ -210,6 +210,7 @@`
		`"srpm_url": "http://example.com/mypkg.src.rpm",`
		`"chroots": chroot_name_list`
		`}`
		`+ self.u1.admin = False`
		`self.db.session.commit()`
		`r0 = self.request_rest_api_with_auth(`
		`"/api_2/builds",`
		`@@ -255,6 +256,7 @@`
		`}`
		`login = self.u2.api_login`
		`token = self.u2.api_token`
		`+ self.u1.admin = False`
		`self.db.session.commit()`
		`r0 = self.request_rest_api_with_auth(`
		`"/api_2/builds",`
		`@@ -458,6 +460,8 @@`
		`bc.status = StatusEnum("pending")`
		`bc.ended_on = None`

		`+ self.u1.admin = False`
		`+`
		`self.db.session.add_all(self.b1_bc)`
		`self.db.session.add(self.b1)`

frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_builds.py

file modified

+3 -2

		`@@ -59,7 +59,7 @@`
		`f_coprs,`
		`f_copr_permissions,`
		`f_db):`
		`-`
		`+ self.u1.admin = False`
		`self.db.session.add_all([self.u1, self.c1])`
		`self.test_client.post("/coprs/{0}/{1}/new_build/"`
		`.format(self.u1.name, self.c1.name),`
		`@@ -111,6 +111,7 @@`
		`for bc in self.b1_bc:`
		`bc.status = StatusEnum("pending")`
		`bc.ended_on = None`
		`+ self.u1.admin = False`
		`self.db.session.add_all(self.b1_bc)`
		`self.db.session.add_all([self.u1, self.c1, self.b1])`
		`self.test_client.post("/coprs/{0}/{1}/cancel_build/{2}/"`
		`@@ -280,7 +281,7 @@`
		`f_coprs,`
		`f_mock_chroots,`
		`f_builds, f_db):`
		`-`
		`+ self.u1.admin = False`
		`self.db.session.add_all([self.u1, self.c1, self.b1])`
		`r = self.test_client.post(`
		`"/coprs/{0}/{1}/repeat_build/{2}/"`

thrnciar commented 4 years ago

fixes #827

praiskup commented on line 6 of frontend/coprs_frontend/coprs/models.py 4 years ago

here would be return True preferred, but looks good to me

praiskup commented 4 years ago

+1 OK we need to fix testsuite first

Edited 4 years ago by praiskup

frostyx commented on line 6 of frontend/coprs_frontend/coprs/models.py 4 years ago

For some reason on many places in that function, we set can_build instead of returning the value immediately. If we want to change this particular occurrence within this PR, I would prefer to change all of them.

frostyx commented on line 5 of frontend/coprs_frontend/coprs/models.py 4 years ago

When testing whether something is true, it is better to do if copr.user.admin is True: instead of ==, or even better if copr.user.admin:

frostyx commented 4 years ago

I made some small suggestions, but overall it looks good.

1 new commit added

frontend: fixes tests and code refactoring

4 years ago

1 new commit added

frontend: fixes wrong condition

4 years ago

thrnciar commented 4 years ago

[copr-build]

rebased onto 1975aef73ee8c8ee078bf5339019051ccc3eaff9

4 years ago

praiskup commented 4 years ago

frostyx commented 4 years ago

I would prefer the commit message starting with [frontend] instead of frontend: as it is a convention for our codebase, but otherwise, it looks good.

Thank you @thrnciar!

rebased onto 8e8867b

4 years ago

Pull-Request has been merged by thrnciar

4 years ago