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

Feature/public event #287

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from

Conversation

noam-y
Copy link
Contributor

@noam-y noam-y commented Feb 14, 2021

a feature that supports describing an event as a public event. as a part of this definition, it will allow sending emails to all event participants and allow joining events on the event page.

@noam-y noam-y changed the base branch from main to develop February 14, 2021 22:26
@@ -61,8 +61,8 @@ class Event(Base):
color = Column(String, nullable=True)
availability = Column(Boolean, default=True, nullable=False)
invitees = Column(String)
is_public = Column(Boolean, default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Event doesn't have "friends" option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the meaning of friends is different- public event is meant to allow any user in the platform to join the event, just like the public events on facebook. so it means the owner's friends don't matter in this feature :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain what is the meaning of public/private event? is it like busy/free?
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a pubic event is event anyone can join. it means you don't need to be invited directly from the owner, you can join the event by yourself by clicking a button on event page



@pytest.fixture
def new_user(session):
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can use existing fixtures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried but didn't really find a proper user fixture inside the file. ill try going over the code again

Copy link
Contributor

Choose a reason for hiding this comment

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

There are enough user fixtures, use one of the others.

app/internal/email.py Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 19, 2021

Codecov Report

Merging #287 (71ee772) into develop (1048956) will decrease coverage by 0.38%.
The diff coverage is 56.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #287      +/-   ##
===========================================
- Coverage    98.01%   97.63%   -0.39%     
===========================================
  Files           68       68              
  Lines         2979     3002      +23     
===========================================
+ Hits          2920     2931      +11     
- Misses          59       71      +12     
Impacted Files Coverage Δ
app/routers/email.py 100.00% <ø> (ø)
app/internal/email.py 84.00% <26.66%> (-16.00%) ⬇️
app/database/models.py 96.73% <100.00%> (+0.03%) ⬆️
app/routers/event.py 96.94% <100.00%> (+0.09%) ⬆️

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 1048956...9cf15b1. 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! Please see my insights, and use precommit hooks to format your code automatically :)

app/routers/event.py Outdated Show resolved Hide resolved
app/routers/event.py Outdated Show resolved Hide resolved
app/routers/event.py Outdated Show resolved Hide resolved
app/templates/eventview.html Outdated Show resolved Hide resolved
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.

Most of the problems in this version could be resolved easily using git precommit hooks.
Please run black or git precommit hooks on this code and I'll check it again :)

app/internal/email.py Outdated Show resolved Hide resolved
app/internal/email.py Outdated Show resolved Hide resolved
app/internal/email.py Show resolved Hide resolved
app/routers/event.py Outdated Show resolved Hide resolved
templates,
)
from app.database.models import Event, User, UserEvent
from app.internal.utils import get_current_user
Copy link
Member

Choose a reason for hiding this comment

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

Please use Kobi's user system instead

app/routers/event.py Outdated Show resolved Hide resolved
app/routers/event.py Outdated Show resolved Hide resolved
app/routers/event.py Outdated Show resolved Hide resolved
tests/test_email.py Outdated Show resolved Hide resolved
tests/test_email.py Outdated Show resolved Hide resolved
Comment on lines 88 to 99
@pytest.fixture
def no_event_user(session):
"""a user made for testing who doesn't own any event."""
user = create_user(
session=session,
username='new_test_username',
password='new_test_password',
email='new2_test.email@gmail.com',
language_id='english'
)

return user
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create a new fixture, use one of the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have to use several different users since i'm testing the use of the mailing list.

Comment on lines 102 to 124
@pytest.fixture
def event_owning_user(session):
"""a user made for testing who already owns an event."""
user = create_user(
session=session,
username='new_test_username2',
password='new_test_password2',
email='new_test_love231.email@gmail.com',
language_id='english'
)

data = {
'title': 'event_owning_user event',
'start': datetime.strptime('2021-05-05 14:59', '%Y-%m-%d %H:%M'),
'end': datetime.strptime('2021-05-05 15:01', '%Y-%m-%d %H:%M'),
'location': 'https://us02web.zoom.us/j/875384596',
'content': 'content',
'owner_id': user.id,
}

create_event(session, **data)

return user
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create a new fixture, use one of the others. You can use a user fixture and an event fixture in your code to create this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have to use several different users since i'm testing the use of the mailing list.

__

Comment on lines 127 to 137
@pytest.fixture
def user1(session):
"""another user made for testing"""
user = create_user(
session=session,
username='user2user2',
password='verynicepass',
email='trulyyours1.email@gmail.com',
language_id='english'
)
return user
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create a new user fixture, use one of the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have to use several different users since i'm testing mailing list.

Comment on lines 140 to 152
@pytest.fixture
def event_example(session, event_owning_user):
data = {
'title': 'test event title',
'start': datetime.strptime('2021-05-05 14:59', '%Y-%m-%d %H:%M'),
'end': datetime.strptime('2021-05-05 15:01', '%Y-%m-%d %H:%M'),
'location': 'https://us02web.zoom.us/j/87538459r6',
'content': 'content',
'owner_id': event_owning_user.id,
}

event = create_event(session, **data)
return event
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create a new event fixture, use one of the others.



@pytest.fixture
def new_user(session):
Copy link
Contributor

Choose a reason for hiding this comment

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

There are enough user fixtures, use one of the others.

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.

None yet

5 participants