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

Adding pagination fo news-flash api #2700

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

ziv17
Copy link
Collaborator

@ziv17 ziv17 commented Sep 9, 2024

No description provided.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 68.42105% with 12 lines in your changes missing coverage. Please review.

Project coverage is 52.77%. Comparing base (400af13) to head (58bf0ec).
Report is 98 commits behind head on dev.

Files with missing lines Patch % Lines
anyway/views/news_flash/api.py 68.42% 12 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2700      +/-   ##
==========================================
- Coverage   53.22%   52.77%   -0.46%     
==========================================
  Files         119      122       +3     
  Lines        9924    10102     +178     
==========================================
+ Hits         5282     5331      +49     
- Misses       4642     4771     +129     
Flag Coverage Δ
unittests 52.77% <68.42%> (-0.46%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ziv17 ziv17 marked this pull request as ready for review September 10, 2024 03:04
@atalyaalon
Copy link
Collaborator

@ziv17 is there a FE change needed once this is merged to prod?

@atalyaalon
Copy link
Collaborator

Also, is this closing this? #2697

@ziv17
Copy link
Collaborator Author

ziv17 commented Sep 15, 2024

@ziv17 is there a FE change needed once this is merged to prod?

No. The new behavior takes place only when one of the new parameters is supplied. As I wrote in the FE issue.

@atalyaalon
Copy link
Collaborator

@ziv17 looks good!
Merging this one, next task in this issue is handling the use case when the news flash id is a param with pagination (I assume we'll need to modify get_news_flash_by_id

@atalyaalon atalyaalon merged commit 67a5c9b into data-for-change:dev Sep 16, 2024
3 checks passed
@ziv17
Copy link
Collaborator Author

ziv17 commented Sep 16, 2024

Also, is this closing this? #2697

It does not handle the newsflash I'd parameter. I will do that in the next PR

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.

3 participants