Investment Project Documents #250
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #250 +/- ##
===========================================
- Coverage 93.65% 93.38% -0.28%
===========================================
Files 56 58 +2
Lines 2049 2115 +66
Branches 190 194 +4
===========================================
+ Hits 1919 1975 +56
- Misses 109 118 +9
- Partials 21 22 +1
Continue to review full report at Codecov.
|
canni
changed the title
config/settings/common.py
Outdated
| @@ -169,3 +170,9 @@ | |||
| CHAR_FIELD_MAX_LENGTH = 255 | |||
| HEROKU = False | |||
| BULK_CREATE_BATCH_SIZE = env.int('BULK_CREATE_BATCH_SIZE', default=50000) | |||
|
|
|||
| AWS_ACCESS = { | |||
There was a problem hiding this comment.
boto3 reads directly from the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables, should we just use those?
Also need to update the README with new environment variables (in the Heroku section...)
[Edit]
Actually, it looks like we're using instance profiles elsewhere which Boto3 will pick up automatically.
config/settings/common.py
Outdated
| 'KEY_ID': env('AWS_KEY_ID'), | ||
| 'KEY_SECRET': env('AWS_KEY_SECRET'), | ||
| } | ||
| DOCUMENTS_BUCKET = env('DOC_BUCKET') |
There was a problem hiding this comment.
Can we keep the name of the Python and environment variable consistent?
datahub/core/utils.py
Outdated
| if not s3: | ||
| s3 = boto3.client( | ||
| 's3', | ||
| region_name='eu-west-2', |
There was a problem hiding this comment.
Would be nice not to hard-code the region. Boto3 will use the AWS_DEFAULT_REGION environment variable.
datahub/core/utils.py
Outdated
|
|
||
| def sign_s3_url(bucket_name, path, method='get_object', expires=3600): | ||
| """Sign s3 url using global config, and given expiry in seconds.""" | ||
| s3 = getattr(sign_s3_url, 's3_instance', None) |
There was a problem hiding this comment.
Might be useful to split the the client part into a separate function (e.g. get_s3_client()).
datahub/documents/models.py
Outdated
| uploaded_on = models.DateTimeField( | ||
| null=True, blank=True | ||
| ) | ||
| av_clean = models.BooleanField(default=False) |
There was a problem hiding this comment.
Do we need to store whether the scan has completed or not as well? Also defaulting to False is a bit confusing, since that sounds like it's infected. NullBooleanField might fix both problems...
There was a problem hiding this comment.
I think something like checked_on would be useful to see performance of the checks and for audit - for example if it turns out file is infected or if we want to check files that has not been checked in let's say two weeks.
datahub/documents/models.py
Outdated
|
|
||
| @property | ||
| def url(self): | ||
| """Generate pre-signed download URL.""" |
There was a problem hiding this comment.
Not too much of a fan of generating pre-signed URLs in properties...
There was a problem hiding this comment.
At least property name should inform that the url is signed?
datahub/investment/models.py
Outdated
| return self.document.upload_url | ||
|
|
||
| class Meta: # noqa: D101 | ||
| verbose_name = 'Investment Project Document' |
There was a problem hiding this comment.
Isn't the verbose name meant to be in lower case?
datahub/investment/views.py
Outdated
|
|
||
|
|
||
| class IProjectDocumentViewSet(CoreViewSetV3): | ||
| """Investment Project Serializer.""" |
There was a problem hiding this comment.
Docstring needs updating here
No description provided.