feat: implement checks on session rating #6070

shreyanshdwivedi · 2019-06-19T06:57:45Z

Fixes fossasia/open-event-frontend#3156

Short description of what this resolves:

There should be checks that feedback can be updated/created only by owner/organizer of the event.

Changes proposed in this pull request:

Updates the typo
Updates docs
Implemented checks on feedback so that it can be updated/created only by owner/organizer of the event.

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.

app/api/feedbacks.py

shreyanshdwivedi · 2019-06-19T07:48:49Z

@uds5501 @mrsaicharan1 @kushthedude can you guys tell me how can I create a session with ID 1 for testing? Please see the travis failure error

app/api/feedbacks.py

mrsaicharan1

@shreyanshdwivedi Please fix Travis. .apib docs file needs to be fixed here

shreyanshdwivedi · 2019-06-20T22:40:14Z

@iamareebjamal how can I create session object for the travis to pass?

iamareebjamal · 2019-06-21T00:17:23Z

factoryboy

shreyanshdwivedi · 2019-06-22T06:38:33Z

@iamareebjamal @uds5501 @mrsaicharan1 @kushthedude fixed the travis failure. Please review

iamareebjamal · 2019-06-22T07:36:21Z

tests/hook_main.py

-        event = EventFactoryBasic()
-        db.session.add(event)
+        feedback = FeedbackFactory()
+        db.session.add(feedback)


You should use SessionFactory

Using SessionFactory is failing tests. Moreover all hooks having creation object e.g. user, event, role etc. are followed similarly. I think it'll be okay to use this only @iamareebjamal
Your views?

You changed event feedback to session feedback. Instead, we are supporting both if I'm not wrong

@iamareebjamal yes we are supporting both but currently only session feedback is being used. Moreover the error raised was related to user, not event.User.id = 2 not found. I'm adding a user attribute to check if error is being fixed or not

@iamareebjamal the tests passed. I used both session and user (which are currently into account). When we will kove to event rating, the tests will be updated and EventFactorybasic will be included. Please have a look now

Instead of breaking a functionality, add it in such a way it does not break previous one. Add a new endpoint in API Blueprints instead of changing existing one

@iamareebjamal event feedback functionality is non-existent.
There was no check or piece of code which was for event rating functionality in our system. There was only feedback schema with both event and session relationships, so I just used it for session rating. When event rating is to be implemented, we can create a separate API blueprint for it at that time.
However if you think that I should revert the changes and create a separate API blueprint, please let me know. I'll make changes accordingly if you want

The same API was being used for event feedback if I'm not wrong

@iamareebjamal actually there is no event feedback in existence AFAIK. However I've asked others on gitter if it exists.
There was only feedback schema and models which were meant for future implementation. They were never used (atleast in my knowledge).
Is android using it already?

tests/hook_main.py

shreyanshdwivedi · 2019-06-23T04:41:42Z

@iamareebjamal I confirmed with others. There is no functionality for event feedback in place currently.
So when event feedback is to be implemented, we can create a new API blueprint for it then

iamareebjamal · 2019-06-23T06:45:37Z

docs/api/api_blueprint.apib

+                  "user": {
+                    "data": {
+                      "type": "user",
+                      "id": "2"


The authenticated user is 1, should be 1 only. It should not be 2

iamareebjamal · 2019-06-23T06:46:33Z

docs/api/api_blueprint.apib

                },
                "type": "feedback",
-                "id": 1,
+                "id": 2,


Since there is no other feedback in system before POST request, this should be 1?

iamareebjamal · 2019-06-23T23:07:14Z

app/factories/feedback.py

-    event_id = 1
+    comment = "Awesome session."
+    session_id = 1
    user_id = 2


user_id 1 is the currently authenticated user. How can user 1 post session for 2? Checks aren't working correctly it seems

Yeah. I somehow missed to set it to 1
But the test passed. That's weird
I'll make it to 1 anyway

@iamareebjamal I've updated it to 1. User with ID 1 can change sessions for 2 because the checks implemented here gives access to someone with organizer access of an event, to create/update feedback for that event -

if session and not has_access('is_organizer', event_id=session.event_id): raise ForbiddenException({'source': ''}, "Event organizer access required")

shreyanshdwivedi · 2019-06-26T06:33:05Z

@iamareebjamal please review

iamareebjamal · 2019-06-26T13:03:01Z

tests/hook_main.py

+        session = SessionFactory()
+        db.session.add(session)
+        user = UserFactory()
+        db.session.add(user)


UserFactory is not needed since there already is a user - 1

Update docs for session rating fixes hound issue fixes travis failure update test hook

feat: implement checks on session rating (fossasia#6070)

houndci-bot reviewed Jun 19, 2019

View reviewed changes

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

auto-label bot added the feature label Jun 19, 2019

iamareebjamal reviewed Jun 19, 2019

View reviewed changes

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

iamareebjamal reviewed Jun 19, 2019

View reviewed changes

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

shreyanshdwivedi force-pushed the rateSession branch from 877576a to b168d68 Compare June 19, 2019 12:39

fossasia deleted a comment Jun 19, 2019

iamareebjamal previously approved these changes Jun 19, 2019

View reviewed changes

uds5501 previously approved these changes Jun 19, 2019

View reviewed changes

kushthedude previously approved these changes Jun 19, 2019

View reviewed changes

mrsaicharan1 suggested changes Jun 20, 2019

View reviewed changes

shreyanshdwivedi dismissed stale reviews from kushthedude, uds5501, and iamareebjamal via af58543 June 21, 2019 08:33

shreyanshdwivedi force-pushed the rateSession branch from b168d68 to af58543 Compare June 21, 2019 08:33

fossasia deleted a comment Jun 21, 2019

shreyanshdwivedi force-pushed the rateSession branch from af58543 to 0ecb230 Compare June 21, 2019 09:13

fossasia deleted a comment Jun 21, 2019

shreyanshdwivedi force-pushed the rateSession branch from 0ecb230 to 3fa0de2 Compare June 21, 2019 09:48

fossasia deleted a comment Jun 21, 2019

shreyanshdwivedi force-pushed the rateSession branch from 3fa0de2 to df71f05 Compare June 21, 2019 10:05

fossasia deleted a comment Jun 21, 2019

shreyanshdwivedi changed the title ~~feat: implement checks on session rating~~ [WIP] feat: implement checks on session rating Jun 21, 2019

auto-label bot removed the feature label Jun 21, 2019

shreyanshdwivedi force-pushed the rateSession branch from df71f05 to aa03061 Compare June 22, 2019 04:30