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.

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

For #430: TableListView test with authentication. #506

Merged
merged 2 commits into from
Mar 11, 2019
Merged

For #430: TableListView test with authentication. #506

merged 2 commits into from
Mar 11, 2019

Conversation

paulodamaso
Copy link
Contributor

For #430: TableListView test with authentication.

  • Fixed test for TableListView mocking authenticated user related to Company, Floor and Location (to make sure that the user has permission to some group of tables)
  • Left puzzle for fixing TableListView, which is broken: it just returns the header of the page

@0crat 0crat added the scope Issue is in scope label Mar 8, 2019
@0crat
Copy link

0crat commented Mar 8, 2019

Job #506 is now in scope, role is REV

@codecov
Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #506 into master will decrease coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
- Coverage   88.35%   87.91%   -0.45%     
==========================================
  Files          55       55              
  Lines        1005     1018      +13     
  Branches       55       57       +2     
==========================================
+ Hits          888      895       +7     
- Misses         96      100       +4     
- Partials       21       23       +2
Impacted Files Coverage Δ
timeless/restaurants/tables/views.py 100% <ø> (ø) ⬆️
timeless/roles/views.py 83.87% <0%> (-8.44%) ⬇️
timeless/auth/views.py 57.5% <0%> (-5%) ⬇️
timeless/reservations/views.py 82.35% <0%> (-0.99%) ⬇️
timeless/access_control/authorization.py 75% <0%> (ø) ⬆️
timeless/restaurants/table_shapes/views.py 100% <0%> (ø) ⬆️
timeless/access_control/owner_privileges.py 90.9% <0%> (+0.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62ded8f...db32739. Read the comment docs.

@0crat
Copy link

0crat commented Mar 8, 2019

This pull request #506 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

Copy link
Contributor

@vryazanov vryazanov left a comment

Choose a reason for hiding this comment

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

@paulodamaso I left a few comments, pls have a look

with client.session_transaction() as session:
session["user_id"] = employee.id
g.user = employee
factories.TableFactory(floor_id=floor.id, name="Table 01")
Copy link
Contributor

Choose a reason for hiding this comment

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

@paulodamaso Factory provides method for bulk creation:
factories.TableFactory.create_batch(size=3, floor_id=floor.id, name="Table 01")

response = client.get("/tables/")
print(response.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

@paulodamaso let's remove print :)

g.user = employee
factories.TableFactory(floor_id=floor.id, name="Table 01")
factories.TableFactory(floor_id=floor.id, name="Table 01")
factories.TableFactory(floor_id=floor.id, name="Table 01")
response = client.get("/tables/")
Copy link
Contributor

Choose a reason for hiding this comment

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

@paulodamaso let's add url_for instead of url. wdyt?

@paulodamaso
Copy link
Contributor Author

@vladarefiev Thanks for the comments, please take a look at the corrections. About the factories.TableFactory.create_batch(size=3, floor_id=floor.id, name="Table 01") usage, I can't use it because I have to know the table names to assert if they are correct later

@vryazanov
Copy link
Contributor

@rultor merge

@rultor
Copy link
Contributor

rultor commented Mar 11, 2019

@rultor merge

@vryazanov Thanks for your request. @emilianodellacasa Please confirm this.

@emilianodellacasa
Copy link
Contributor

@rultor merge

@rultor
Copy link
Contributor

rultor commented Mar 11, 2019

@rultor merge

@emilianodellacasa OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit db32739 into timelesslounge:master Mar 11, 2019
@rultor
Copy link
Contributor

rultor commented Mar 11, 2019

@rultor merge

@emilianodellacasa Done! FYI, the full log is here (took me 8min)

@0crat 0crat removed the scope Issue is in scope label Mar 11, 2019
@0crat
Copy link

0crat commented Mar 11, 2019

@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

@0crat
Copy link

0crat commented Mar 11, 2019

Job #506 is not in the agenda of @vryazanov/z, can't inspect

@0crat
Copy link

0crat commented Mar 11, 2019

The job #506 is now out of scope

@0crat
Copy link

0crat commented Mar 11, 2019

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @emilianodellacasa/z

@ypshenychka
Copy link

@vryazanov According to our QA Rules:

The code reviewer found at least three problems in the code.
Comments were mostly about design problems, not cosmetic issues.

Only two issues were found during code review.
Please confirm that you'll try to find at least three major problems while future reviews.

@ypshenychka
Copy link

@vryazanov ping
@emilianodellacasa I believe you will need to say "quality acceptable" here. Thanks

@vryazanov
Copy link
Contributor

@ypshenychka @emilianodellacasa I confirm

@emilianodellacasa
Copy link
Contributor

@0crat quality acceptable

@0crat
Copy link

0crat commented Mar 15, 2019

Quality review completed: +4 point(s) just awarded to @emilianodellacasa/z

@0crat
Copy link

0crat commented Mar 15, 2019

Order was finished, quality is "acceptable": +15 point(s) just awarded to @vryazanov/z

@ypshenychka
Copy link

@emilianodellacasa It's still in my agenda

@emilianodellacasa
Copy link
Contributor

@0crat refuse

@0crat
Copy link

0crat commented Mar 16, 2019

@0crat refuse (here)

@emilianodellacasa Job gh:timelesslounge/timelessis#506 is not assigned, can't get performer

@emilianodellacasa
Copy link
Contributor

@ypshenychka now?

@ypshenychka
Copy link

@emilianodellacasa Still on the list...

@emilianodellacasa
Copy link
Contributor

@0crat out

@emilianodellacasa
Copy link
Contributor

@ypshenychka going trial-and-error here :-) now?

@0crat
Copy link

0crat commented Mar 16, 2019

@0crat out (here)

@emilianodellacasa Job gh:timelesslounge/timelessis#506 is not assigned, can't get performer

@0crat
Copy link

0crat commented Mar 16, 2019

This job is not in scope

@emilianodellacasa
Copy link
Contributor

@0crat status

@0crat
Copy link

0crat commented Mar 16, 2019

@0crat status (here)

@emilianodellacasa This is what I know about this job in CF9LG1YCA, as in §32:

@emilianodellacasa
Copy link
Contributor

@ypshenychka should be ok now, confirm?

@ypshenychka
Copy link

@emilianodellacasa Still there
image

@emilianodellacasa
Copy link
Contributor

@ypshenychka can you wait the issue?

@ypshenychka
Copy link

@emilianodellacasa It's not bothering me, but would be nice if it goes away from my agenda someday :)

@emilianodellacasa
Copy link
Contributor

@ypshenychka It would be better to open a new issue on zerocracy and ask for advice to solve current situation, what do you think?

@ypshenychka
Copy link

@emilianodellacasa Good idea. Will do. Thanks

@ypshenychka
Copy link

@emilianodellacasa BTW my agenda is now clear. Not sure how it happened, but issue is not actual anymore :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants