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

Feature/document status check #376

Merged
merged 19 commits into from
May 16, 2024
Merged

Feature/document status check #376

merged 19 commits into from
May 16, 2024

Conversation

252afh
Copy link
Contributor

@252afh 252afh commented May 15, 2024

Context

Documents being uploaded should update their status as it goes through the system.

Changes proposed in this pull request

  • Use polling from JS to get django to poll core for info
  • Altered the JS to use the json response
  • Added client call for getting file status from core
  • Added a view to django to get file_status from core or try use s3 file status if present or return unknown

Guidance to review

  • Go to documents page
  • Upload a document
  • See the status change in the file list

To test error states

  • Change the ID used in the file status endpoint to send the file id instead of the core file id
  • See that the file status changes to unknown

Relevant links

Things to check

  • I have added any new ENV vars in all deployed environments
  • I have tested any code added or changed
  • I have run integration tests

Copy link
Contributor

@brunns brunns left a comment

Choose a reason for hiding this comment

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

Looks good. Might be worth rebasing over main and using the core_file_uuid from the File model now we are collecting it, and commit when it's fully working.

from redbox_app.redbox_core import models


class ChunkStatus:
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels like DRF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 50/50 on adding the extra weight that adding DRF will do, I'm hoping that this will be the only non-view url in the django app

Copy link
Collaborator

Choose a reason for hiding this comment

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

fair point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fully agree though that if we even think we'll add more in the future then we should add DRF, I'll raise it at tomorrows stand-up

@252afh 252afh force-pushed the feature/document-status-check branch from 56b1d31 to d0a2c7c Compare May 15, 2024 15:01
@252afh 252afh marked this pull request as ready for review May 15, 2024 18:03
@@ -104,7 +112,7 @@ def upload_view(request):
)


def injest_file(uploaded_file: UploadedFile, user: User) -> list[str]:
def ingest_file(uploaded_file: UploadedFile, user: User) -> list[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Doh! Sorry about that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this was my fault

Copy link
Contributor

@brunns brunns left a comment

Choose a reason for hiding this comment

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

Post MVP it might be worth considering a streaming approach here rather than a polling one. Most of the pieces are in place for this on the Django side, put there for the streaming chat.

django_app/redbox_app/redbox_core/views.py Outdated Show resolved Hide resolved
django_app/redbox_app/redbox_core/client.py Outdated Show resolved Hide resolved
@252afh 252afh merged commit f5ce122 into main May 16, 2024
8 of 9 checks passed
@252afh 252afh deleted the feature/document-status-check branch May 16, 2024 11:00
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