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

2742 telegram images are blurry and out of order #2756

Conversation

tkalir
Copy link
Collaborator

@tkalir tkalir commented Jan 5, 2025

No description provided.

@tkalir tkalir force-pushed the 2742-telegram-images-are-blurry-and-out-of-order branch from fb57716 to 214cf20 Compare January 5, 2025 14:34
@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 10.00000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 52.50%. Comparing base (400af13) to head (214cf20).
Report is 151 commits behind head on dev.

Files with missing lines Patch % Lines
anyway/telegram_accident_notifications.py 11.11% 32 Missing ⚠️
anyway/infographic_image_generator.py 0.00% 4 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2756      +/-   ##
==========================================
- Coverage   53.22%   52.50%   -0.72%     
==========================================
  Files         119      122       +3     
  Lines        9924    10246     +322     
==========================================
+ Hits         5282     5380      +98     
- Misses       4642     4866     +224     
Flag Coverage Δ
unittests 52.50% <10.00%> (-0.72%) ⬇️

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.

@atalyaalon atalyaalon merged commit e9dfa95 into data-for-change:dev Jan 8, 2025
3 checks passed
@atalyaalon
Copy link
Collaborator

@tkalir I did merge but after another review I think some improvements are needed, hence not merging to master yet.
I think that fetch_widgets_with_retries shouldn't use the requests with our infographics URL.
Please consult with @ziv17 - I think you should use the same function flask app is using to extract the widgets data.
@ziv17 can you review?

@tkalir
Copy link
Collaborator Author

tkalir commented Jan 8, 2025

@atalyaalon
The reason I used the API is
1 - we already used it in the telegram flow to get the transcriptions
2 - the code in the "/api/infographics-data" endpoint uses infographics_data_by_location() which currently references a request object in its implementation. meaning the code will need some refactoring to be able to run outside of an endpoint.

I could do this refactoring, but actually, if we go with the front-end changes that I am thinking of, it will make this change unnecessary. I can elaborate on these changes if you want to talk about it before we are talking with front.

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.

None yet

3 participants