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

fix: Disallow attendee editing if order status not initializing #7085

Merged
merged 8 commits into from
Jun 29, 2020

Conversation

Haider8
Copy link
Contributor

@Haider8 Haider8 commented Jun 27, 2020

Fixes #7066

Short description of what this resolves:

Attendee details can be edited even if the status of the corresponding order is not initializing.

Changes proposed in this pull request:

Attendee can no longer be edited if the order status is not initializing.

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.

@auto-label auto-label bot added the fix label Jun 27, 2020
@@ -206,6 +206,12 @@ def before_update_object(self, obj, data, kwargs):
:param kwargs:
:return:
"""
order = safe_query(Order, 'id', obj.order_id, 'id')
Copy link
Member

Choose a reason for hiding this comment

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

Use new method, safe_query_by_id

order = safe_query(Order, 'id', obj.order_id, 'id')
if order.status != 'initializing':
raise UnprocessableEntityError(
{'pointer': '/data/id'}, "Order status is not initializing",
Copy link
Member

Choose a reason for hiding this comment

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

This is not proper error. You should say that attendee can't be updated and why

@iamareebjamal
Copy link
Member

Add a test for the appropriate cases, like places, completed, pending.

Also, attendee should not be able to be deleted as well once it is created

if order.status != 'initializing':
raise UnprocessableEntityError(
{'pointer': '/data/id'},
"Attendee can't be updated because the corresponding order is not in initializing state",

Choose a reason for hiding this comment

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

line too long (105 > 90 characters)

@Haider8
Copy link
Contributor Author

Haider8 commented Jun 28, 2020

Also, attendee should not be able to be deleted as well once it is created

@iamareebjamal Yes, right now we can't delete attendee if order status is initializing, but the error message is same as that of edit attendee.

@iamareebjamal
Copy link
Member

OK. Please fix dredd tests and add new tests

{
'data': {
'type': 'attendee',
'id': str(attendee.id),
Copy link
Member

Choose a reason for hiding this comment

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

You should modify some attributes here and


# Attendee should not be updated
assert response.status_code == 422
assert attendee.order.id == attendee_order.id
Copy link
Member

Choose a reason for hiding this comment

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

Check here that those attributes aren't modified. You copied a test which was testing that order was not modified. That isn't needed here


# Attendee should not be updated
assert response.status_code == 422
assert attendee.order.id == attendee_order.id
Copy link
Member

Choose a reason for hiding this comment

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

Same

@@ -46,7 +46,7 @@
from tests.factories.session import SessionFactory
from tests.factories.speaker import SpeakerFactory
from tests.factories.ticket import TicketFactory
from tests.factories.attendee import AttendeeFactory
from tests.factories.attendee import AttendeeFactory, AttendeeOrderSubFactory

Choose a reason for hiding this comment

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

module level import not at top of file

@iamareebjamal
Copy link
Member

Great, now let's just hope dredd works

@Haider8
Copy link
Contributor Author

Haider8 commented Jun 29, 2020

It will work

@niranjan94
Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- app/api/attendees.py  1
         

Clones added
============
- tests/all/integration/api/attendee/test_attendee_api.py  2
         

See the complete overview on Codacy

@codecov
Copy link

codecov bot commented Jun 29, 2020

Codecov Report

Merging #7085 into development will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #7085   +/-   ##
============================================
  Coverage        62.00%   62.01%           
============================================
  Files              262      262           
  Lines            12992    12995    +3     
============================================
+ Hits              8056     8059    +3     
  Misses            4936     4936           
Impacted Files Coverage Δ
app/api/attendees.py 50.42% <100.00%> (+1.28%) ⬆️

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 9a38212...05be9e8. Read the comment docs.

@iamareebjamal
Copy link
Member

Working. Saw old status

@iamareebjamal iamareebjamal merged commit 1169f10 into fossasia:development Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallow attendee editing after order is in pending+ state
4 participants