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

Upload documents to google drive (UD3, UD6.1, UD6.2) #1327

Merged
merged 4 commits into from
Feb 18, 2020

Conversation

Renny4Real
Copy link
Contributor

@Renny4Real Renny4Real commented Feb 17, 2020

This PR is the rebased form of #1302 and will replace it.
(Pushed the rebased branch to a separate branch to check that the rebase worked without losing our original work)

Cancel functionality has been broken out into a new ticket (TEVA-504)

Google drive issue has been documented in a separate ticket (TEVA-505)

Uploading progress indicator to be reviewed, amended and finalised (TEVA-506)

ACCEPTANCE (TEVA-408):

  • Show users a list of documents they have uploaded
  • While a file is uploading, I see a 'cancel' control
  • The 'Save and continue' button is deactivated when files are uploading. It is reactivated when files have finished uploading
  • If the file upload is complete, show the user the file size
  • Once a file has finished uploading, its status shows as 'Uploaded'
  • If the file upload is not complete, show the user an upload percentage indicator. Collective indicator of all upload steps.
  • A happy path set of tests exist that ensures this interaction occurs
  • The code has been peer-reviewed and approved by at least two other developers
  • A running example of this workflow has been signed off by Product, UX
  • The code is released to live

@Renny4Real Renny4Real changed the title Upload documents to google drive (UD3) Upload documents to google drive (UD3, UD6.1, UD 6.2) Feb 18, 2020
@Renny4Real Renny4Real changed the title Upload documents to google drive (UD3, UD6.1, UD 6.2) Upload documents to google drive (UD3, UD6.1, UD6.2) Feb 18, 2020
@Renny4Real Renny4Real marked this pull request as ready for review February 18, 2020 09:36
@cpjmcquillan cpjmcquillan requested a review from tatyree February 18, 2020 10:17
Copy link
Contributor

@tatyree tatyree left a comment

Choose a reason for hiding this comment

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

Overall good, but needs a bit of work on a few small things. See comments.

app/frontend/src/uploadDocuments.js Outdated Show resolved Hide resolved
app/frontend/src/uploadDocuments.js Outdated Show resolved Hide resolved
app/frontend/src/uploadDocuments.js Outdated Show resolved Hide resolved
app/frontend/src/uploadDocuments.js Outdated Show resolved Hide resolved
app/frontend/src/uploadDocuments.js Outdated Show resolved Hide resolved
app/services/document_upload.rb Show resolved Hide resolved
end

def file_size_error(filename)
@errors = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a risk, in using a separate variable to track whether there are errors, that it will become decoupled from the actual state of @documents_form.errors which is partly determined by ActiveModel.

unless @errors 

could become (something like):

unless @documents_form.errors

in order to have a single source of truth about whether there are errors. Maybe this is just a pattern I'm unaware of though.

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 not sure, but I think this is because we don't validate whether we have a virus in the form model.
We wouldn't know if there was a virus unless Google tells us so the method is invoked depending on the response from Google.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the error is created on line 84:

@documents_form.errors.add(:base, t('jobs.file_virus_error_message', filename: filename))

Does that count as validating in the form model, for your purposes?

Copy link
Contributor

@tatyree tatyree left a comment

Choose a reason for hiding this comment

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

:shipit:

@cpjmcquillan cpjmcquillan merged commit e19d982 into develop Feb 18, 2020
@cpjmcquillan cpjmcquillan deleted the upload-documents-to-gdrive-rebased branch February 18, 2020 16:10
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.

6 participants