#181 handling owner location access #304
Conversation
|
Job #304 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #304 +/- ##
==========================================
+ Coverage 81.39% 81.87% +0.47%
==========================================
Files 42 42
Lines 758 756 -2
Branches 48 49 +1
==========================================
+ Hits 617 619 +2
+ Misses 122 118 -4
Partials 19 19
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #304 +/- ##
==========================================
+ Coverage 81.39% 81.57% +0.18%
==========================================
Files 42 42
Lines 758 760 +2
Branches 48 48
==========================================
+ Hits 617 620 +3
Misses 122 122
+ Partials 19 18 -1
Continue to review full report at Codecov.
|
|
This pull request #304 is assigned to @vryazanov/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @emilianodellacasa/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job |
| status="status" | ||
| ) | ||
| db_session.add(location) | ||
| db_session.commit() |
There was a problem hiding this comment.
@krzyk let's add instances to session and then make one db_session.commit(), instead of multiple commits
There was a problem hiding this comment.
@krzyk and the same in the second test
There was a problem hiding this comment.
@vryazanov not quite, I get id's only after I do commit, so e.g. if I want to use my_company.id, I need to commit it first
There was a problem hiding this comment.
@krzyk Actually to get instance id need to execute db_session.flush(), it will interact with database, but transaction will not be commited. but anyway, I think we can leave it as it is
| # @todo #181:30min This test stopped working because it was not working | ||
| # according to requirements. | ||
| # Write new tests that validate 'is_allowed' according to specifications in #22 | ||
| @pytest.mark.skip("fix me") | ||
| def test_owner_can_access_location(): | ||
| assert ( |
There was a problem hiding this comment.
@krzyk let's make it simpler:
assert is_allowed(method=Method.DELETE, resource="location")There was a problem hiding this comment.
@vryazanov is it really necessary? I didn't touch this test and this whole test should be removed altogether, because it doesn't make sense after the changes. We should write new ones
| return permitted | ||
| user_company = flask.g.get("user").company_id | ||
| location_company = Location.query.get(kwargs.get("id")).company_id | ||
| if method in (Method.READ, Method.CREATE, Method.UPDATE, Method.DELETE) \ |
There was a problem hiding this comment.
@krzyk you listed all possible methods here :)
Seems this code does the same:
user_company = flask.g.get("user").company_id
location_company = Location.query.get(kwargs.get("id")).company_id
return user_company == location_company
|
@vryazanov updated and replied |
|
@rultor merge |
@vryazanov Thanks for your request. @emilianodellacasa Please confirm this. |
|
@rultor merge |
@emilianodellacasa OK, I'll try to merge now. You can check the progress of the merge here |
@emilianodellacasa Done! FYI, the full log is here (took me 7min) |
|
Job was finished in 16 hours, bonus for fast delivery is possible (see §36) |
|
@ypshenychka/z please review this job completed by @vryazanov/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #304 is now out of scope |
|
Payment to |
|
@vryazanov According to our QA Rules:
Only one issue was found during code review. |
|
@ypshenychka I confirm |
|
@vryazanov thanks |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +20 point(s) just awarded to @vryazanov/z |
|
Quality review completed: +8 point(s) just awarded to @ypshenychka/z |
#181