Uploading spreadsheet for updates #15

aaclan-ebi · 2021-07-06T11:21:04Z

broker/service/spreadsheet_storage/spreadsheet_storage_service.py

broker/service/spreadsheet_upload_service.py

test/service/test_spreadsheet_generator.py

amnonkhen · 2021-07-07T13:54:04Z

test/service/test_spreadsheet_generator.py

+    def tearDown(self) -> None:
+        if self.test_file:
+            if os.path.exists(self.test_file):


I think you could use the tempfile library for tempfiles that automatically delete

cool, very nice lib! thanks!

test/test_broker_app.py

test/service/test_spreadsheet_upload_service.py

ke4

LGTM

amnonkhen · 2021-07-08T11:03:02Z

broker/service/spreadsheet_upload_service.py

+            raise SpreadsheetUploadError(401, "An authentication token must be supplied when uploading a spreadsheet")
+        self.ingest_api.set_token(token)
+
+    def upload(self, submission_url, path, project_uuid=None):


Is upload for a full project rather than just updates?

The upload function is for uploading spreadsheets with new entities. Sometime's the project is created before submission so we supply it if it already exists.

amnonkhen · 2021-07-08T11:03:59Z

broker/service/spreadsheet_upload_service.py

+            submission_resource = self.ingest_api.create_submission()
+        return submission_resource
+
+    def _assert_token_exists(self, token):


Is this input validation code? assert makes it sound like testing code.

Renamed to _validate_token_exists

amnonkhen · 2021-07-08T11:10:23Z

broker_app.py

-ingest_api = IngestApi()
-spreadsheet_generator = SpreadsheetGenerator(ingest_api)
-spreadsheet_job_manager = SpreadsheetJobManager(spreadsheet_generator, SPREADSHEET_STORAGE_DIR)
+global ingest_api


Is using global a safe approach?
Should we use flask's app context instead? or the "g" object?

Ah, i didn't know about that. Applied.

Hmm it doesn't function as a global variable.

This is a good place to store resources during a request. During testing, you can use the Faking Resources and Context pattern to pre-configure such resources.

I'll revert to using the global :(

It might be my lack of knowledge, but why are we defining these 3 variables here and once again at the setup method?

amnonkhen · 2021-07-08T11:13:54Z

test/integration/test_spreadsheet_generator.py

@@ -12,7 +13,13 @@
 import tempfile
 import yaml

+
+# TODO these are integration tests, could either move to a separate directory


I removed the comments, I already moved them now to a separate directory to make it easier to run tests locally 😅

This reverts commit e7b8bda.

ke4

I made a tiny comment. Could we clarify it, please?
Tx

ke4 · 2021-07-09T09:50:42Z

broker_app.py

-ingest_api = IngestApi()
-spreadsheet_generator = SpreadsheetGenerator(ingest_api)
-spreadsheet_job_manager = SpreadsheetJobManager(spreadsheet_generator, SPREADSHEET_STORAGE_DIR)
+global ingest_api


It might be my lack of knowledge, but why are we defining these 3 variables here and once again at the setup method?

aaclan-ebi · 2021-07-09T10:24:01Z

@ke4 i think you're right, they're not needed. I removed them and moved the setup function on top to make it clearer there are global vars to be initialised. I don't know how else to avoid using global vars at the moment. But, I can see the performance benefit of it that it only initialises the schema template obj which is being used in template spreadsheet generation only once instead of initialising it every request so I'd like to retain the use of global vars for now.

amnonkhen · 2021-07-09T12:25:49Z

broker_app.py


    try:
        logger.info('Uploading spreadsheet!')
-        submission_resource = spreadsheet_upload_svc.async_upload(token, request_file, is_update, project_uuid)
+        submission_resource = spreadsheet_upload_svc.async_upload(token, request_file, is_update, project_uuid, submission_uuid)


The is_update param is just passed through here, and there is subsequent branching logic further down the execution sequence.
This smells of unclean design. Maybe there would be room here for the "strategy" pattern.

I'm afraid I don't have time to implement that for this sprint.

amnonkhen · 2021-07-09T12:26:44Z

requirements.txt

@@ -8,7 +8,7 @@ itsdangerous==0.24
 jdcal==1.4
 MarkupSafe==1.1
 werkzeug>=0.15.3
-e git+https://github.com/ebi-ait/ingest-client.git@7f04ab9a#egg=hca_ingest
+-e git+https://github.com/ebi-ait/ingest-client.git@7038dcb#egg=hca_ingest


Why are we not using a normal version number? (x.y.z)

This was done this way in the early development of the ingest-client python lib. It wasn't changed yet. We should also set up and release build for the ingest-client repo so that it'll release the module to PyPI every merges to master.

I filed a ticket for the ingest-client CI/CD setup: ebi-ait/dcp-ingest-central#385

test/integration/test_spreadsheet_generator.py

aaclan-ebi · 2021-07-09T12:57:16Z

Hi @amnonkhen , I would like to merge this today so that I can deploy and hand over to a wrangler for testing, we could apply those comments later.

broker_app.py

aaclan-ebi · 2021-07-13T10:47:20Z

Closing in favour of PR #16 . This branch was already merged with that branch.

aaclan-ebi changed the title ~~Uploading spreadsheet for updates, failing tests~~ Uploading spreadsheet for updates Jul 6, 2021

Uploading spreadsheet for updates, failing tests

0f510d8

aaclan-ebi force-pushed the feature/spreadsheet-updates branch from 478968a to 0f510d8 Compare July 6, 2021 11:23

Alegria Aclan added 4 commits July 6, 2021 12:35

Passing of submission url

c947a80

Renamed method in the importer

247a886

Optimise imports

6987fef

Reverted requirements.txt upgrades

7dc73f9

aaclan-ebi force-pushed the feature/spreadsheet-updates branch from 75cf343 to 7dc73f9 Compare July 6, 2021 17:57

aaclan-ebi changed the base branch from master to dev July 6, 2021 17:58

aaclan-ebi self-assigned this Jul 7, 2021

Alegria Aclan added 2 commits July 7, 2021 10:11

Fix tests

ee084b8

Updated tests

b41fc75

aaclan-ebi requested a review from a team July 7, 2021 11:06

aaclan-ebi marked this pull request as ready for review July 7, 2021 11:06

amnonkhen reviewed Jul 7, 2021

View reviewed changes

broker/service/spreadsheet_storage/spreadsheet_storage_service.py Show resolved Hide resolved

Make sure directory exists before saving the file

0eb1118

aaclan-ebi requested a review from a team July 7, 2021 13:04

Use makedirs not mkdir

4e3148b

aaclan-ebi force-pushed the feature/spreadsheet-updates branch from 767d4e4 to 4e3148b Compare July 7, 2021 13:24

amnonkhen requested changes Jul 7, 2021

View reviewed changes

Alegria Aclan added 4 commits July 7, 2021 20:56

Use tempfile for deleting files in the tests

4b79c12

Move redundant code to setUp function

9a2ac4c

Refactored async_upload for readability

ee6a0a1

Remove brittle assertions

ce2ece9

aaclan-ebi requested a review from a team July 7, 2021 20:34

Alegria Aclan added 2 commits July 7, 2021 21:45

Better function name

3480393

Some more fixes to tests

b8ec29e

ke4 previously approved these changes Jul 8, 2021

View reviewed changes

amnonkhen requested changes Jul 8, 2021

View reviewed changes

amnonkhen previously approved these changes Jul 8, 2021

View reviewed changes

Use latest version of ingest-client

a953c09

aaclan-ebi dismissed stale reviews from amnonkhen and ke4 via a953c09 July 8, 2021 12:33

Alegria Aclan added 10 commits July 8, 2021 13:35

Rename function for clarity

1f23747

Remove comments as they're already applied

c1e1132

Reverted use of global declaration and setup

e7b8bda

Use flask g application context instead of global variables

bca3780

Revert "Reverted use of global declaration and setup"

f96bf7e

This reverts commit e7b8bda.

Remove g usage in tests

449b977

Remove g usage, revert commit was insufficient

acfcfac

Added __init__.py to run nosetests

5899ce0

Remove multiprocessing as it has a very short time out

485398a

Use latest ingest-client version - fixed bug

4286224

ke4 reviewed Jul 9, 2021

View reviewed changes

Move setup to top

f0c0dee

amnonkhen requested changes Jul 9, 2021

View reviewed changes

amnonkhen requested changes Jul 12, 2021

View reviewed changes

broker_app.py Show resolved Hide resolved

broker_app.py Show resolved Hide resolved

amnonkhen approved these changes Jul 13, 2021

View reviewed changes

aaclan-ebi closed this Jul 13, 2021

MightyAx deleted the feature/spreadsheet-updates branch October 5, 2022 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uploading spreadsheet for updates #15

Uploading spreadsheet for updates #15

aaclan-ebi commented Jul 6, 2021 •

edited

Loading

amnonkhen Jul 7, 2021

aaclan-ebi Jul 7, 2021

ke4 left a comment

amnonkhen Jul 8, 2021

aaclan-ebi Jul 8, 2021

amnonkhen Jul 8, 2021

aaclan-ebi Jul 8, 2021

amnonkhen Jul 8, 2021

aaclan-ebi Jul 8, 2021

aaclan-ebi Jul 8, 2021

ke4 Jul 9, 2021

amnonkhen Jul 8, 2021

aaclan-ebi Jul 8, 2021

ke4 left a comment

ke4 Jul 9, 2021

aaclan-ebi commented Jul 9, 2021 •

edited

Loading

amnonkhen Jul 9, 2021

aaclan-ebi Jul 9, 2021 •

edited

Loading

amnonkhen Jul 9, 2021

aaclan-ebi Jul 9, 2021

aaclan-ebi Jul 9, 2021

aaclan-ebi Jul 13, 2021

aaclan-ebi commented Jul 9, 2021

aaclan-ebi commented Jul 13, 2021

Uploading spreadsheet for updates #15

Uploading spreadsheet for updates #15

Conversation

aaclan-ebi commented Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ke4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ke4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaclan-ebi commented Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

aaclan-ebi Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaclan-ebi commented Jul 9, 2021

aaclan-ebi commented Jul 13, 2021

aaclan-ebi commented Jul 6, 2021 •

edited

Loading

aaclan-ebi commented Jul 9, 2021 •

edited

Loading

aaclan-ebi Jul 9, 2021 •

edited

Loading