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: Add endpoint for upcoming events #7049

Merged
merged 5 commits into from
Jun 28, 2020

Conversation

mansiag
Copy link
Contributor

@mansiag mansiag commented Jun 5, 2020

Fixes #6955

Short description of what this resolves:

The filtering for upcoming events is done on the frontend using the filter query parameter. This should be handled by the server, so an endpoint /events/upcoming is created to automatically return the upcoming events.

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.

query_ = self.session.query(Event).filter(
Event.ends_at > current_time,
Event.state=='published',
Event.privacy=='public').order_by(Event.starts_at)

Choose a reason for hiding this comment

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

missing whitespace around operator

current_time = datetime.now(pytz.utc)
query_ = self.session.query(Event).filter(
Event.ends_at > current_time,
Event.state=='published',

Choose a reason for hiding this comment

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

missing whitespace around operator

else:
self.schema = EventSchemaPublic

self.schema.self_view_many = 'v1.upcoming_event_list'
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already defined in superclass?

'session': db.session,
'model': Event,
'methods': {'query': query},
}
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 already defined in superclass as well I believe

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 query method is not invoked for returning the desired events without defining data_layer again in the child class. It should take methods in data_layer variable from superclass, but this isn't happening.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. The library is not really well maintained. OK

Copy link
Member

Choose a reason for hiding this comment

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

Check other things. If they are working or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, other things are working fine.

@iamareebjamal
Copy link
Member

Please also add a dredd test in the docs to ensure it works and conforms to the docs

@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #7049 into development will increase coverage by 0.00%.
The diff coverage is 54.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #7049   +/-   ##
============================================
  Coverage        61.15%   61.16%           
============================================
  Files              260      260           
  Lines            12881    12894   +13     
============================================
+ Hits              7878     7887    +9     
- Misses            5003     5007    +4     
Impacted Files Coverage Δ
app/api/events.py 22.19% <44.44%> (+0.48%) ⬆️
app/api/routes.py 100.00% <100.00%> (ø)
app/api/helpers/scheduled_jobs.py 50.00% <0.00%> (+1.70%) ⬆️

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 98aca7d...3a10c5c. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jun 5, 2020

This pull request fixes 1 alert when merging aa23466 into 98aca7d - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@mansiag
Copy link
Contributor Author

mansiag commented Jun 6, 2020

Please also add a dredd test in the docs to ensure it works and conforms to the docs

I am quite confused if the URL parameters filter and sort should still be added to the docs, as one may want to further filter these events ?
Also, sort parameter will be of no use, as we are performing sort in the server code, should sorting be removed from the server and moved to the frontend?

@iamareebjamal
Copy link
Member

You just need to add the URL without filter and sorting

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jun 7, 2020

This pull request fixes 1 alert when merging 73a6f97 into 98e6c5c - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@iamareebjamal
Copy link
Member

Actual response returned no events https://app.apiary.io/public/tests/run/acb5ac2f-cba0-44d1-bd68-48248bc5ede0

@iamareebjamal
Copy link
Member

Also, remove the page size and number

:return:
"""
current_time = datetime.now(pytz.utc)
query_ = self.session.query(Event).filter(
Copy link
Member

Choose a reason for hiding this comment

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

This does not filter deleted events. Maybe that is done automatically in the library, but can you please still verify that deleted events are not being returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, deleted events are not returned.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jun 7, 2020

This pull request fixes 1 alert when merging d2484ae into 98e6c5c - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@mansiag
Copy link
Contributor Author

mansiag commented Jun 7, 2020

@iamareebjamal
Copy link
Member

Working fine. Can you please change PR title to match semantic guidelines?

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jun 7, 2020

This pull request fixes 1 alert when merging 3a10c5c into 98e6c5c - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@mansiag mansiag changed the title Create API endpoint /events/upcoming for Upcoming Events feat: Add endpoint for upcoming events Jun 8, 2020
@auto-label auto-label bot added the feature label Jun 8, 2020
@iamareebjamal iamareebjamal merged commit 83dad01 into fossasia:development Jun 28, 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.

Filter upcoming events on the basis of their end date and time.
3 participants