Use STATIC_URL for a few images #80

Merged
merged 1 commit into from Apr 2, 2016

Conversation

Projects
None yet
2 participants
@atodorov
Contributor

atodorov commented Mar 23, 2016

Also fix comments_count by casting the primary key value to string because the dict structure containing the values uses string keys, not integers.

NOTE: There are quire a few static images assigned dynamically from the JavaScript files or from CSS styles. I can make these work by treating JS and CSS as templates and using the STATIC_URL variable. This is dependent on django-compressor, which in turn needs a recent Django version (see my other PR).

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 28, 2016

Member

Well I do have a few issues with this SQL hotfix

this isn't the proper way to do DB schema changes in Django in the first place,

Yes, I agree. Since Nitrate has a long development history, many of aspects of Nitrate looks not good enough. Regarding to the DB schema migration, we had tried South, however, in practice, it's a little bit difficult to use and maintain migration modules among developers. So, we switched to such SQL files holding the migrations.

After upgrading to newer version of Django, it would be good to convert all these necessary migrations to Django migrations carefully.

regardless of that I don't see the upgrade procedure documented so it's easy to miss applying the changes in this file

Here is the document of setting development environment, https://github.com/Nitrate/Nitrate/blob/develop/docs/source/set_dev_env.rst

Anyway, this would be not much visible to be discovered.

I have deployed a fresh version of Nitrate, not upgraded from a previous one, so no DB migrations were actually performed. Note: my deployment date is after the release date of this hotfix so I just did ./manage.py syncdb

At this moment, for either local development or an instance to run Nitrate, it's recommended to initialize database by loading the SQL dump file.

Let's move the schema migrations forward step by step carefully.

All of this means that I still have this column as string, so the cast is still needed. I can however, split this into a separate patch and apply the cast only if needed.

Thank you very much. :)

Member

tkdchen commented Mar 28, 2016

Well I do have a few issues with this SQL hotfix

this isn't the proper way to do DB schema changes in Django in the first place,

Yes, I agree. Since Nitrate has a long development history, many of aspects of Nitrate looks not good enough. Regarding to the DB schema migration, we had tried South, however, in practice, it's a little bit difficult to use and maintain migration modules among developers. So, we switched to such SQL files holding the migrations.

After upgrading to newer version of Django, it would be good to convert all these necessary migrations to Django migrations carefully.

regardless of that I don't see the upgrade procedure documented so it's easy to miss applying the changes in this file

Here is the document of setting development environment, https://github.com/Nitrate/Nitrate/blob/develop/docs/source/set_dev_env.rst

Anyway, this would be not much visible to be discovered.

I have deployed a fresh version of Nitrate, not upgraded from a previous one, so no DB migrations were actually performed. Note: my deployment date is after the release date of this hotfix so I just did ./manage.py syncdb

At this moment, for either local development or an instance to run Nitrate, it's recommended to initialize database by loading the SQL dump file.

Let's move the schema migrations forward step by step carefully.

All of this means that I still have this column as string, so the cast is still needed. I can however, split this into a separate patch and apply the cast only if needed.

Thank you very much. :)

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 30, 2016

Contributor

Updated the PR to fix only STATIC_URL usage so it can be easily applied. I will open another PR for the object_pk cast to string.

Contributor

atodorov commented Mar 30, 2016

Updated the PR to fix only STATIC_URL usage so it can be easily applied. I will open another PR for the object_pk cast to string.

@tkdchen tkdchen merged commit e4ceed8 into Nitrate:develop Apr 2, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 2, 2016

Member

Merged. Thanks.

Member

tkdchen commented Apr 2, 2016

Merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment