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

[back][front] refactor: remove obsolete /video/ routes #1963

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

GresilleSiffle
Copy link
Collaborator

@GresilleSiffle GresilleSiffle commented Apr 30, 2024

related issues #1867


Description

To close the milestone "API responses refactor" this PR removes some obsolete code in the back end and the front end.

(a) In the back end the following endpoints have been deleted:

  • GET /video/ (replaced by GET /polls/{name}/recommendations/)
  • GET /video/{video_id} (replaced by GET /polls/{name}/entities/{uid})

According to our logs, the only 3 requests to GET /video/ during the last 30 days have been made by robots.

# use the following query in Grafana
{filename="/var/log/nginx/json_access.log"} | json | http_host = "api.tournesol.app" | request_uri = "/video/" | request_method = "GET"

(b) In the front end the CriteriaRadarChart has been removed. This component is unused and depends on the deleted back end serializer VideoSerializerWithCriteria.

Checklist

  • I added the related issue(s) id in the related issues section (if any)
    • if not, delete the related issues section
  • I described my changes and my decisions in the PR description
  • I read the development guidelines of the CONTRIBUTING.md
  • The tests pass and have been updated if relevant
  • The code quality check pass

@GresilleSiffle GresilleSiffle added Backend Back-end code of Tournesol Frontend Front-end code of Tournesol labels Apr 30, 2024
@GresilleSiffle GresilleSiffle marked this pull request as ready for review May 6, 2024 13:44
@GresilleSiffle GresilleSiffle self-assigned this May 6, 2024
Copy link
Member

@amatissart amatissart left a comment

Choose a reason for hiding this comment

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

It looks good to me.

But a call to GET /video/ persists in "data-visualisation":

response = requests.get(
f"https://api.tournesol.app/video/?limit=99999&unsafe=true"
).json()

So we may want to update that first (and trying to load all videos in a single request is probably no longer a good idea).

Comment on lines 92 to 96
# TODO: restore this check after the refactor of POST /video/
#
# GET requests must not be throttled, even if previous POST requests has been
response = client.get("/video/")
self.assertEqual(response.status_code, status.HTTP_200_OK)
# response = client.get("/video/")
# self.assertEqual(response.status_code, status.HTTP_200_OK)
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed now than only POST are supported by this route.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed by 6e8b194

@GresilleSiffle
Copy link
Collaborator Author

So we may want to update that first (and trying to load all videos in a single request is probably no longer a good idea).

Exactly. I created the PR #1969 to address this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend Back-end code of Tournesol Frontend Front-end code of Tournesol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants