-
Notifications
You must be signed in to change notification settings - Fork 1
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
Imgee V2 #59
Conversation
iambibhas
commented
Apr 18, 2017
•
edited
Loading
edited
- Removes celery from the system
- Removes unnecessary code and a lot of cleanup
- Replaced previous tests. Previously the tests were making requests to the HTTP endpoints. But some of those endpoints are authenticated. And no way to test those endpoints right now. Not sure how they used to work before. So now we moved most of the view logic to separate functions and testing those.
- Added support for new file types - PSD, AI, SVG, PDF, XCF, WEBP and CDR.
Removed celery as dependency
New tests
some more cleanups
imgee/models/stored_file.py
Outdated
if new_labels != old_labels: | ||
self.labels = labels | ||
|
||
if (new_labels == old_labels): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brackets aren't really necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed them.
tests/test_upload.py
Outdated
return sf | ||
|
||
def test_save_file(self): | ||
resp = requests.get("http:" + os.path.join(app.config.get('MEDIA_DOMAIN'), app.config.get('AWS_FOLDER'), "{}{}".format(self.sf.name, self.sf.extn))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a method that gives you the path for a filename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fixed this.
imgee/views/files.py
Outdated
from sqlalchemy import and_ | ||
|
||
from coaster.views import load_model, load_models | ||
from imgee import app, forms, lastuser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe import just the relevant classes and functions from forms
, instead of importing everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use forms
for from baseframe import forms
, so it may be confusing to someone reading this file and wondering what the forms
namespace is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial comments. Going through the rest.
.gitignore
Outdated
*.bak | ||
instance/production.env.sh | ||
imgee/static/gen | ||
tests/imgee/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What goes in this folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uploads made during running the tests.
-> % tree tests/imgee
tests/imgee
└── static
└── test_uploads
├── 0a795f311f90494292df03f64317c57d.svg
├── 1ce47cefeec241b9a7266ed852131e48.svg
├── 28029c34ba9947ad8e6ce53e074b3617.gif
├── 318c1dbbefa7451e82fee1d7c77e3282.svg
├── 31b9a8a9be6947db9913fdb38c0fd4e2.jpeg
.travis.yml
Outdated
- nosetests --with-coverage | ||
install: | ||
install: | ||
- sudo apt-get -qq install zlib1g-dev libfreetype6-dev liblcms1-dev libwebp-dev libjpeg-dev libpng-dev libfreetype6-dev libtiff4-dev librsvg2-dev ghostscript imagemagick pandoc | ||
- pip install -r requirements.txt | ||
- pip install nose coverage BeautifulSoup4 Pillow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are BS and Pillow only required for tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, no. Neither of them are being used now. Removing.
connection = op.get_bind() | ||
Session = sessionmaker(bind=connection.engine) | ||
session = Session(bind=connection) | ||
imgs = session.query(StoredFile).filter_by(size=None).options(load_only("id", "name", "title")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe Alembic gives you a session already. Is a new session necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jace can do that with op.execute()
but then have to define the schema for stored file in this migration file itself. I wanted to avoid it. so creating a session and using that so that I can use the ORM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a problem. Migrations must never import models. They must always be locally defined in the migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that the model may no longer exist in the same form in the code when the migration is run. You need version controlling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jace that's exactly why I did this. The way it was before, the model was imported and used which was breaking the migrations. The way above, it uses the session of the migration, so it uses the version of the model at that point in migration and not the current version of the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't make sense. You're importing from models. You always get the current version of the model.
imgee/__init__.py
Outdated
from .async import TaskRegistry | ||
from .tasks import TaskRegistry | ||
|
||
registry = TaskRegistry(os.getenv('ENV', 'production')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any dependency on system environment should be in coaster.app
. The system environment is not a secure place for keeping settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I set this inside the init_for(env)
function and then use env? TaskRegistry
has name='default'
set by default. I wanted to use something more verbose for the redis keys. hence did this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcode name='imgee'
?
imgee/models/stored_file.py
Outdated
def dict_data(self): | ||
return dict( | ||
title=self.title, | ||
uploaded=self.created_at.strftime('%B %d, %Y'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use .isoformat() + 'Z'
imgee/storage.py
Outdated
img_name = scaled.name | ||
else: | ||
size = get_fitting_size((img.width, img.height), size_t) | ||
resized_filename = get_resized_filename(img, size) | ||
job = queueit('resize_and_save', img, size, is_thumbnail=is_thumbnail, taskid=resized_filename) | ||
return job | ||
registry = imgee.registry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is a repeat of the one above.
imgee/storage.py
Outdated
from imgee.utils import ( | ||
newid, guess_extension, get_file_type, | ||
path_for, get_s3_folder, get_s3_bucket, | ||
download_frm_s3, get_width_height, ALLOWED_MIMETYPES, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be named download_from_s3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Fixing.
imgee/storage.py
Outdated
img_name = resize_and_save(img, size, is_thumbnail=is_thumbnail) | ||
except Exception as e: | ||
# something broke while processing the file | ||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This except
block is redundant. You only need the finally
block.
imgee/storage.py
Outdated
@@ -101,13 +120,13 @@ def save_on_s3(filename, remotename='', content_type='', bucket='', folder=''): | |||
k = b.new_key(folder+filename) | |||
headers = { | |||
'Cache-Control': 'max-age=31536000', # 60*60*24*365 | |||
'Content-Type': get_file_type(fp), | |||
'Content-Type': get_file_type(fp, filename), | |||
'Expires': datetime.now() + timedelta(days=365) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use utcnow()
. Also, don't you need to convert to string?
imgee/storage.py
Outdated
@@ -150,7 +171,11 @@ def get_fitting_size((orig_w, orig_h), size): | |||
>>> get_fitting_size((200, 500), (400, 600)) | |||
[240, 600] | |||
""" | |||
if size[0] == 0 and size[1] == 0: | |||
if orig_w == 0 or orig_h == 0: | |||
# this is either a cdr file or a zero width file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a "zero width" file? Technically that can't be an image. Helps to clarify which of the following is the case:
- We are unable to parse this image to extract its size.
- It's a vector image with no size in pixels. However, it will still have an aspect ratio that matters when resizing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 is the practical case. We're unable to parse the image's width and height.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments.
imgee/storage.py
Outdated
|
||
@celery.task(name='imgee.storage.delete', base=BaseTask) | ||
def delete(stored_file): | ||
def delete(stored_file, commit=True): | ||
""" | ||
Delete all the thumbnails and images associated with a file, from local cache and S3. | ||
Wait for the upload/resize to complete if queued for the same image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the waiting happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The waiting was there with celery where this function will just wait if a resize operation is going on. Right now there is no wait. The delete function will delete everything even if something is going on parallel. What should be the ideal flow of this? Let's saw a resize task is going on. There is an entry for the file in taskregistry. what should the delete function do?
db.session.commit() | ||
|
||
if commit: | ||
db.session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there was a background thumbnail task? Will it continue running and finish generating a thumbnail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question above. I can add few(2-3 maybe) seconds of waiting to delete function to wait for any resize task to be finished. But what if the resize isn't done by that time? What should the delete function do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's file this as a separate ticket and move on. If a user uploads an image and immediately deletes it, while a thumbnail render is still in progress, does it clean up properly?
imgee/tasks.py
Outdated
self.connection = redis.from_url(connection) | ||
|
||
def add(self, taskid): | ||
self.connection.sadd(self.key, taskid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a timed key (Redis only supports TTL on top-level keys), so there is the possibility of a task (a) getting stuck and running longer than a timeout period, or (b) crashing and not cleaning up, so it appears to still be running. Where's the cleanup process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a timestamp along with this key and then clear that ourselves inside the get_resized_image()
function?
imgee/utils.py
Outdated
from subprocess import check_output, CalledProcessError | ||
from urlparse import urljoin | ||
from uuid import uuid4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use uuid1mc
from coaster.utils
wherever possible. uuid4
has bad database indexing performance because of the randomness.
connection = op.get_bind() | ||
Session = sessionmaker(bind=connection.engine) | ||
session = Session(bind=connection) | ||
imgs = session.query(StoredFile).filter_by(size=None).options(load_only("id", "name", "title")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a problem. Migrations must never import models. They must always be locally defined in the migration.
connection = op.get_bind() | ||
Session = sessionmaker(bind=connection.engine) | ||
session = Session(bind=connection) | ||
imgs = session.query(StoredFile).filter_by(size=None).options(load_only("id", "name", "title")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that the model may no longer exist in the same form in the code when the migration is run. You need version controlling.
) * function name fix, using uuid1mc, time format fix * deleting keys from registry when deleting a file * restructured TaskRegistry to use Redis Keys with TTL set to 60 seconds instead of assigning them to a set * re-adding nose and coverage to travis file, removed it by mistake * using runtests.py file to run tests in travis * reordered functions and better string formating * made the redis key expiry time an argument * assigning error handler the right way * not resizing if the gif image is animated * added pillow as dependency * using redis pipeline to delete multiple keys * missed one place to use redis pipeline * deleted runtests.py, not needed anymore * fixed typo * fixed some import issues * moved upload directory creation to manage.py * added some comments * moved importing app inside the only function that uses it * Update runtests.sh * added manage.py init to runtests * removed mkdir from travis config * fixed upload path issue * moar fixes to registry, cleanup, moved back to uuid4 * fixed typo * setting test environment variable in travis file * setting env in existing block, otherwise it gets overwritten * moved the init command out of if block * rearranged travis config, cleanup of upload directory path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First set of comments. Have to finish review for the rest.
connection = op.get_bind() | ||
Session = sessionmaker(bind=connection.engine) | ||
session = Session(bind=connection) | ||
imgs = session.query(StoredFile).filter_by(size=None).options(load_only("id", "name", "title")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't make sense. You're importing from models. You always get the current version of the model.
imgee/__init__.py
Outdated
from .async import TaskRegistry | ||
from .tasks import TaskRegistry | ||
|
||
registry = TaskRegistry(os.getenv('ENV', 'production')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcode name='imgee'
?
imgee/__init__.py
Outdated
@@ -40,14 +37,12 @@ def error403(error): | |||
def init_for(env): | |||
coaster.app.init_app(app, env) | |||
baseframe.init_app(app, requires=['baseframe', 'picturefill', 'imgee']) | |||
app.error_handlers[403] = error403 | |||
app.error_handlers[403] = error403 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this Flask's recommended way of defining an error handler? If not, we should use the recommended way.
imgee/__init__.py
Outdated
@@ -40,14 +37,12 @@ def error403(error): | |||
def init_for(env): | |||
coaster.app.init_app(app, env) | |||
baseframe.init_app(app, requires=['baseframe', 'picturefill', 'imgee']) | |||
app.error_handlers[403] = error403 | |||
app.error_handlers[403] = error403 | |||
lastuser.init_app(app) | |||
lastuser.init_usermanager(UserManager(db, models.User)) | |||
if app.config.get('MEDIA_DOMAIN') and ( | |||
app.config['MEDIA_DOMAIN'].startswith('http:') or | |||
app.config['MEDIA_DOMAIN'].startswith('https:')): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs, startswith
takes a tuple. You can use: app.config['MEDIA_DOMAIN'].startswith(('http:', 'https:'))
imgee/__init__.py
Outdated
celery.conf.add_defaults(app.config) | ||
registry.set_connection() | ||
app.register_blueprint(api, url_prefix='/api/1') | ||
app.upload_folder = os.path.join(app.static_folder, app.config.get('UPLOADED_FILES_DIR')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use werkzeug's secure_filename here.
imgee/views/files.py
Outdated
if form.validate_on_submit(): | ||
file_name = request.form.get('file_name') | ||
q = and_(Profile.userid.in_(g.user.user_organizations_owned_ids()), StoredFile.name == file_name) | ||
f = StoredFile.query.filter(q).first_or_404() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query is broken. It does not check for any association between StoredFile
and Profile
. Any user's file can be edited. Security breach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query you want:
if profile.userid not in g.user.user_organizations_owned_ids():
abort(403)
f = StoredFile.query.filter_by(profile=profile, name=file_name).first_or_404()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the edit
permission is granted correctly in the Profile
model, the abort
check may be unnecessary. Confirm that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jace how do I confirm the permissions granted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examine g.permissions
in the debugger. Look up the permissions
methods on the Profile
and StoredFile
models to see if either of them grants edit
and if so, under what conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jace does this look right?
ipdb> g.permissions
set(['edit', 'new-file', 'new-label', u'siteadmin', 'new', 'view', 'delete'])
ipdb> profile.permissions(g.user)
set(['edit', 'new-file', 'new-label', 'new', 'view', 'delete'])
How to check under what condition
part?
imgee/views/files.py
Outdated
@lastuser.requires_login | ||
@load_models( | ||
(Profile, {'name': 'profile'}, 'profile'), | ||
(StoredFile, {'name': 'file'}, 'stored_file'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security breach unless you use (StoredFile, {'name': 'file', 'profile': 'profile'}, 'stored_file'),
imgee/views/files.py
Outdated
permission=['edit', 'siteadmin']) | ||
def update_title_json(profile, stored_file): | ||
form = UpdateTitle() | ||
if form.validate_on_submit(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security breach unless you use:
if profile.userid not in g.user.user_organizations_owned_ids():
abort(403)
if form.validate_on_submit():
This is also subject to how the edit
permission is granted.
imgee/views/files.py
Outdated
permission=['delete', 'siteadmin']) | ||
def delete_file(profile, img): | ||
form = DeleteImageForm() | ||
if form.is_submitted(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use form.validate_on_submit()
to protect against CSRF.
imgee/views/files.py
Outdated
@load_models( | ||
(Profile, {'name': 'profile'}, 'profile'), | ||
(StoredFile, {'name': 'image', 'profile': 'profile'}, 'img'), | ||
permission=['delete', 'siteadmin']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm the delete
permission is granted correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review part two of two.
imgee/views/index.py
Outdated
@load_model(Profile, {'name': 'profile'}, 'profile', | ||
permission=['view', 'siteadmin'], addlperms=lastuser.permissions) | ||
permission=['view', 'siteadmin']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need the addlperms
parameter to receive siteadmin
. It's needed anywhere you're checking for the siteadmin
permission.
imgee/views/index.py
Outdated
def pop_up_gallery(profile): | ||
label = request.args.get('label') | ||
files = profile.stored_files | ||
if label: | ||
files = files.join(StoredFile.labels).filter(Label.name==label) | ||
files = files.join(StoredFile.labels).filter(Label.name == label) | ||
files = files.order_by('stored_file.created_at desc').all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to files.order_by(db.desc(StoredFile.created_at))
imgee/views/index.py
Outdated
title_form = forms.EditTitleForm() | ||
return render_template('profile.html', profile=profile, files=files, title_form=title_form, unlabelled=True) | ||
|
||
|
||
def get_prev_next_images(profile, img, limit=2): | ||
# query for "all" images though we need just the `limit` | ||
# bcoz we don't know how many are there in deleteQ. | ||
imgs = profile.stored_files.order_by('created_at desc').all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query is terribly inefficient. You should ask the database to tell you what's previous and next, and return only those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prev = profile.stored_files.filter(StoredFile.created_at <= img.created_at, StoredFile.id != img.id).order_by('created_at desc').limit(limit).all()
next = profile.stored_files.filter(StoredFile.created_at >= img.created_at, StoredFile.id != img.id).order_by('created_at').limit(limit).all()
imgee/views/labels.py
Outdated
if saved: | ||
db.session.commit() | ||
status = {'+': ('Added', 'to'), '-': ('Removed', 'from'), '': ('Saved', 'to')} | ||
plural = 's' if len(saved) > 1 else '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ngettext
for plural text. Don't do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use separate strings for each action so they can be localized properly. Guide to ngettext
at https://pythonhosted.org/Flask-BabelEx/#using-translations
imgee/views/labels.py
Outdated
@@ -104,26 +115,7 @@ def utils_save_label(label_name, profile, commit=True): | |||
|
|||
|
|||
def utils_delete_label(label): | |||
if isinstance(label, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basestring
, not str
runtests.sh
Outdated
#!/bin/sh | ||
set -e | ||
export FLASK_ENV="TESTING" | ||
export PYTHONPATH=$(dirname $0) # setting project root as PYTHONPATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary anymore?
return dict( | ||
title=self.title, | ||
uploaded=self.created_at.isoformat() + 'Z', | ||
filesize=app.jinja_env.filters['filesizeformat'](self.size), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just call this function directly? Unless it's an inbuilt Jinja filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an inbuilt filter of jinja.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it needs a binary
parameter too. https://github.com/pallets/jinja/blob/master/jinja2/filters.py#L459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the binary format? By default it's False
.
|
||
if resized_filename in registry: | ||
# file is still being processed | ||
# returning None will show "no preview available" thumbnail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should distinguish between "no preview available" and "loading". Latter should show a spinner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only have one scenario right now, what to show when an image is processing. so maybe instead of "no preview available" let's just show "loading" spinner. The no preview case might be possible if a file fails to process. but that needs some better handling I think. can I file that as a separate ticket and make this no-preview thumbnail to loading spinner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
tn = Thumbnail(name=name, width=tn_w, height=tn_h, stored_file=img) | ||
db.session.add(tn) | ||
db.session.commit() | ||
if Thumbnail.query.filter(Thumbnail.name == name).isempty(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these name
fields here? On the Thumbnail model and in the tn_name
parameter? Please add comments to clarify. This may also need a separate ticket to determine if we're doing this correctly. For example, it should be possible to make both JPEG and WebP thumbnails of the same image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thumbnail model only saves the name part of the thumbnail, without extension. So right now it's not possible to have multiple thumbnails of different types. can be implemented. separate ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, separate ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A sample of what's in name
as a comment in the code will help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments.
o = check_output('identify -quiet -ping -format "%wx%h" {}'.format(img_path), shell=True) | ||
return tuple(int(dim) for dim in o.split('x')) | ||
if extn in ['.pdf', '.gif']: | ||
o = check_output('identify -quiet -ping -format "%wx%h" {}[0]'.format(img_path), shell=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move all these commands into a single place? Having them littered around code like this makes it hard to find. This can be a separate ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these commands are just to figure out the resolutions of images, and they are only inside this function. can move them somewhere else and pick one depending on file types but filetypes like cdr requires 2 commands to figure out the size, so not sure. Can turn this function to a ResolutionDetector class. That'll be more organized i think, but not sure if that'll be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File a ticket and return to this later.
website.py
Outdated
@@ -1,4 +1,4 @@ | |||
import sys | |||
import os.path | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're using nothing else from the os
module, import os.path
. It's the recommended approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in all the places we're using just os.path
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will push.
* removed celery as dependency * fixed - was missing folder name when downloading file * handling multiple resize request for same file * removed print statements * if there is an error while processing a file, raise it * fixed comment * removed all tests * removed more unnecessary things * removed unnecessary imports and minor cleanup * renamed async.py * added label tests * separating label logic * fixed label tests * more cleanup, moved some views out of index.py * cleaned up label saving codes a little * added tests for upload and delete * updated status code for delete test * mixed couple of migrations and showing error * some more cleanups * using get_image_url to create image url * not showing delete label button if not logged in * importing only the needed forms * added tests for resizing files and testing with multiple file types * comparing redirect location with generated thumbnail url * added redis to travis * fixed fixtures file to use new init_app behavior * Function name fix, using uuid1mc, time format fix, TaskRegistry fix (#60) * function name fix, using uuid1mc, time format fix * deleting keys from registry when deleting a file * restructured TaskRegistry to use Redis Keys with TTL set to 60 seconds instead of assigning them to a set * re-adding nose and coverage to travis file, removed it by mistake * using runtests.py file to run tests in travis * reordered functions and better string formating * made the redis key expiry time an argument * assigning error handler the right way * not resizing if the gif image is animated * added pillow as dependency * using redis pipeline to delete multiple keys * missed one place to use redis pipeline * deleted runtests.py, not needed anymore * fixed typo * fixed some import issues * moved upload directory creation to manage.py * added some comments * moved importing app inside the only function that uses it * Update runtests.sh * added manage.py init to runtests * removed mkdir from travis config * fixed upload path issue * moar fixes to registry, cleanup, moved back to uuid4 * fixed typo * setting test environment variable in travis file * setting env in existing block, otherwise it gets overwritten * moved the init command out of if block * rearranged travis config, cleanup of upload directory path * lot of minor fixes * using dotted relative import syntax everywhere, added tests for registry(redis) * cleaning up queries and imports * cleanup of os.path and more comments on thumbnail * fixed typo * added buildspec.yml file for testing AWS CodeBuild * setting env varialbe manually for aws codebuild * removed buildspec.yml file * showing loading spinner if file is still processing