Skip to content

Conversation

nadav-pesach
Copy link
Contributor

change owner of an event

@codecov-io
Copy link

codecov-io commented Feb 12, 2021

Codecov Report

Merging #268 (63fddba) into develop (8cd3a6e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #268   +/-   ##
========================================
  Coverage    99.04%   99.05%           
========================================
  Files           52       52           
  Lines         2409     2425   +16     
========================================
+ Hits          2386     2402   +16     
  Misses          23       23           
Impacted Files Coverage Δ
app/routers/event.py 97.88% <100.00%> (+0.19%) ⬆️

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 8cd3a6e...63fddba. Read the comment docs.

Copy link
Member

@yammesicka yammesicka left a comment

Choose a reason for hiding this comment

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

Great job! I've added few insights :)

async def changeowner(request: Request, event_id: int,
db: Session = Depends(get_db)):
data = await request.form()
if 'username' in data:
Copy link
Member

Choose a reason for hiding this comment

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

Prefer if 'username' not in form: return [...] and the rest of the code unindented

@router.post("/{event_id}")
async def changeowner(request: Request, event_id: int,
db: Session = Depends(get_db)):
data = await request.form()
Copy link
Member

Choose a reason for hiding this comment

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

Prefer a better name, form for example

</li>
<li class="nav-item">
<!-- new button for the agenda page. should be available after login
<div class="collapse navbar-collapse" id="navbarText">
Copy link
Member

Choose a reason for hiding this comment

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

Return this to be indentation of 2 spaces

@@ -1,338 +1,316 @@
{% extends "base.html" %} {% include "event/partials/text_editor_partial_head.html" %} {% block content %}

<div class="container mt-4">
<div class="container mt-4">
Copy link
Member

Choose a reason for hiding this comment

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

Return to 2 spaces

Copy link
Member

@yammesicka yammesicka left a comment

Choose a reason for hiding this comment

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

Great job! Just few additional small fixes

@@ -97,6 +97,31 @@ async def eventview(request: Request, event_id: int,
"messages": messages})


@router.post("/{event_id}/owner_changed")
Copy link
Member

Choose a reason for hiding this comment

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

Just /owner is good enough :)

@@ -97,6 +97,31 @@ async def eventview(request: Request, event_id: int,
"messages": messages})


@router.post("/{event_id}/owner_changed")
async def changeowner(request: Request, event_id: int,
Copy link
Member

Choose a reason for hiding this comment

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

change_owner

</div>
<div class="event_info_row">
<span class="icon">ICON</span>
<time datetime="{{ event.start }}">{{ event.start.strftime(start_format )}}</time>
-
<time datetime="{{ event.start }}">{{ event.start.strftime(start_format )}}</time> -
Copy link
Member

Choose a reason for hiding this comment

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

Fix the spacing here

@yammesicka yammesicka merged commit 3299fd6 into PythonFreeCourse:develop Feb 15, 2021
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.

3 participants