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

Fix channel thumbnail upload #1140

Merged
merged 3 commits into from
Dec 21, 2018

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Dec 20, 2018

Description

Fixes the behaviour for saving thumbnail_encoding data to the server. As we are using a patch update for this, it does not get passed through the 'toJSON' method of the Channel Model on the frontend, and so is not getting JSON.stringified. Does away with the need for this by storing this data on the backend as a postgres JSONField.

Before/After Screenshots (if applicable)

Before:
channelthumbnailbefore gif

After:
channelthumbnailafter

Steps to Test

  • Open the channel details tab
  • Click 'Edit Details'
  • Upload a new thumbnail
  • Save your changes
  • See no flappy bird/async error

Checklist

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are the migrations safe for a large db (if applicable)?

Comments

Need to test the last checklist item before merging. This involves individual operations on each channel, so may be very slow.

Add tests for data migration.
Change frontend behaviour as no need to serialize and deserialize JSON.
to handle it now being a JSONField.
Add tests to cover all these references.
@rtibbles
Copy link
Member Author

Looks like I was forced to follow the advice for altering a column:
Change the type of a column | Add a new column, change the code to write to both columns, and backfill the new column

I don't think we'll lose data for not making the code write to both the columns, because the bug is preventing this writing anyway, so it will not take effect until it is deployed.

@lyw07
Copy link
Contributor

lyw07 commented Dec 21, 2018

Deployed the code to develop studio.
Got this error when running python manage.py migrate:

root@develop-studio-986877f88-ljcz8:/contentcuration# python contentcuration/manage.py migrate       
Operations to perform:
  Apply all migrations: admin, auth, authtoken, content, contentcuration, contenttypes, sessions, sites
Running migrations:
  Applying contentcuration.0003_copy_data...Traceback (most recent call last):
  File "contentcuration/manage.py", line 15, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/__init__.py", line 364, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/__init__.py", line 356, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/commands/migrate.py", line 204, in handle
    fake_initial=fake_initial,
  File "/usr/local/lib/python2.7/dist-packages/django/db/migrations/executor.py", line 115, in migrate
    state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial)
  File "/usr/local/lib/python2.7/dist-packages/django/db/migrations/executor.py", line 145, in _migrate_all_forwards
    state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
  File "/usr/local/lib/python2.7/dist-packages/django/db/migrations/executor.py", line 244, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/usr/local/lib/python2.7/dist-packages/django/db/migrations/migration.py", line 129, in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
  File "/usr/local/lib/python2.7/dist-packages/django/db/migrations/operations/special.py", line 193, in database_forwards
    self.code(from_state.apps, schema_editor)
  File "/contentcuration/contentcuration/contentcuration/migrations/0003_copy_data.py", line 13, in forwards
    channel.thumbnail_encoding_json = json.loads(channel.thumbnail_encoding) if channel.thumbnail_encoding else {}
  File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 380, in raw_decode
    obj, end = self.scan_once(s, idx)
ValueError: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)

Copy link
Contributor

@micahscopes micahscopes left a comment

Choose a reason for hiding this comment

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

This is really thorough, Richard!

💯

@rtibbles
Copy link
Member Author

image

@rtibbles rtibbles merged commit d72123d into learningequality:master Dec 21, 2018
@rtibbles rtibbles deleted the channel_thumbnails branch December 21, 2018 20:47
@micahscopes
Copy link
Contributor

The most interesting pull request in the world!

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