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

feat: Implement locking a session feature by organizer #5931

Merged
merged 2 commits into from May 27, 2019

Conversation

@shreyanshdwivedi
Copy link
Member

commented May 19, 2019

Fixes #5930
Related PR : fossasia/open-event-frontend#2964

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

Short description of what this resolves:

This feature will lock a session and associated speaker which will prevent her/him from further editing a session. It'll involve adding a new attribute isLocked to session which will be initially set to fals

Changes proposed in this pull request:

  • Adds is_locked to session schema
  • Adds a migration file to update db
  • Adds a check to raise error if locked session is edited
@codecov

This comment has been minimized.

Copy link

commented May 19, 2019

Codecov Report

Merging #5931 into development will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5931      +/-   ##
===============================================
- Coverage         66.4%   66.38%   -0.02%     
===============================================
  Files              285      285              
  Lines            13893    13903      +10     
===============================================
+ Hits              9225     9230       +5     
- Misses            4668     4673       +5
Impacted Files Coverage Δ
app/api/schema/sessions.py 86.84% <100%> (+0.17%) ⬆️
app/models/session.py 90.32% <100%> (+0.21%) ⬆️
app/api/sessions.py 41.17% <28.57%> (-0.79%) ⬇️

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 e261d23...42f6a14. Read the comment docs.

app/api/sessions.py Outdated Show resolved Hide resolved

@shreyanshdwivedi shreyanshdwivedi force-pushed the shreyanshdwivedi:lockSession branch from 2c629d0 to bd4089c May 20, 2019

@shreyanshdwivedi

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

:param view_kwargs:
:return:
"""
if session.is_locked and data.get('is_locked') == session.is_locked:

This comment has been minimized.

Copy link
@uds5501

uds5501 May 20, 2019

Contributor

I am a bit sceptical about this condition, does an already unlocked session gets locked after this?

This comment has been minimized.

Copy link
@shreyanshdwivedi

shreyanshdwivedi May 20, 2019

Author Member

No. This condition is to raise error if a locked session is edited anyhow.
I added this condition data.get('is_locked') == session.is_locked to session.is_locked because if I used session.is_locked alone, error was raised even when I changed state of session's is_locked, which makes sense.

This comment has been minimized.

Copy link
@uds5501

uds5501 May 20, 2019

Contributor

@shreyanshdwivedi Okay, makes sense. But wouldn't toggling the lock button go through this and hence raise a forbidden error upon unlocking?

This comment has been minimized.

Copy link
@shreyanshdwivedi

shreyanshdwivedi May 20, 2019

Author Member

That's why I used data.get('is_locked') == session.is_locked because when we'll be toggling the is_locked property, this condition will never be satisfied @uds5501

This comment has been minimized.

Copy link
@uds5501

uds5501 May 20, 2019

Contributor

@shreyanshdwivedi Understood.

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal May 22, 2019

Member

If data.get('is_locked') is None, this will never be true and error won't be raised. Logic is wrong

Also, what's the point of locking a session if you can unlock it and then edit. Shouldn't the unlocking only be allowed for an admin?

@uds5501
Copy link
Contributor

left a comment

LGTM

:return:
"""
if session.is_locked and data.get('is_locked') == session.is_locked:
raise ForbiddenException({'source': ''}, "Locked sessions cannot be edited")

This comment has been minimized.

Copy link
@CosmicCoder96

CosmicCoder96 May 20, 2019

Collaborator

Point the source to the isLocked property like it is done for marshmallow validations.

This comment has been minimized.

Copy link
@shreyanshdwivedi

shreyanshdwivedi May 20, 2019

Author Member

@CosmicCoder96 I've updated it. Please review

@shreyanshdwivedi shreyanshdwivedi force-pushed the shreyanshdwivedi:lockSession branch from 2b007bd to afa16a3 May 20, 2019

@shreyanshdwivedi

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

@iamareebjamal

This comment has been minimized.

Copy link
Member

commented May 21, 2019

But this only adds the attribute. Where's the logic?

@shreyanshdwivedi

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

@iamareebjamal we don't actually need any logic here. Front-end PR is handling other implementation - fossasia/open-event-frontend#2964.
We only need to add the is_locked attribute and raise error if a locked session is being edited here.

@CosmicCoder96
Copy link
Collaborator

left a comment

@shreyanshdwivedi adding to what @iamareebjamal said, you have made it so that only organizer and admin has access to the frontend routes which allow locking and unlocking sessions. This does not mean that the user can't do that through a simple request directly to the API.
It's a security loophole.

app/api/sessions.py Outdated Show resolved Hide resolved

@shreyanshdwivedi shreyanshdwivedi force-pushed the shreyanshdwivedi:lockSession branch from 991508e to 87f379b May 22, 2019

@shreyanshdwivedi

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

@iamareebjamal @CosmicCoder96 I think data.get(is_locked) will never be None as I've specifically added a check on the schema that it cannot be nullable and by default, the is_locked attribute will be false.

Also, I've just added a check which will ensure that is_locked can be updated only by admins or organizers. Please review

"""
if data.get('is_locked') != session.is_locked:
if not (has_access('is_admin') or has_access('is_organizer')):
raise ForbiddenException({'source': ''}, "Access Forbidden")

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal May 22, 2019

Member

Add proper source and message should be, 'You don't have enough permissions to change this property'

This comment has been minimized.

Copy link
@shreyanshdwivedi

shreyanshdwivedi May 22, 2019

Author Member

@iamareebjamal made the requested changes. Please review

@shreyanshdwivedi shreyanshdwivedi force-pushed the shreyanshdwivedi:lockSession branch 2 times, most recently from d171c72 to 2274e02 May 22, 2019

adds is_locked attribute to sessions schema
adds a migration file for adding is_locked attribute to session

Adds a check to raise error is locked session is edited

Updates source to forbidden exception

check to ensure is_locked can be updated only by admin/organizer

@shreyanshdwivedi shreyanshdwivedi force-pushed the shreyanshdwivedi:lockSession branch from 2274e02 to d4fd1a8 May 22, 2019

@iamareebjamal iamareebjamal changed the title [WIP] feat: Implement locking a session feature by organizer feat: Implement locking a session feature by organizer May 22, 2019

@auto-label auto-label bot added the feature label May 22, 2019

@shreyanshdwivedi

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@CosmicCoder96 please review

@kushthedude
Copy link
Contributor

left a comment

LGTM!

@shreyanshdwivedi

This comment has been minimized.

Copy link
Member Author

commented May 26, 2019

@CosmicCoder96 please review this. Corresponding frontend PR is already merged

@iamareebjamal iamareebjamal merged commit 2ccffb4 into fossasia:development May 27, 2019

3 of 5 checks passed

codecov/patch 50% of diff hit (target 66.4%)
Details
codecov/project 66.38% (-0.02%) compared to e261d23
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shreyanshdwivedi shreyanshdwivedi deleted the shreyanshdwivedi:lockSession branch Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.