Skip to content

WL-327: Add SiteConfiguration and SiteConfigurationHistory models to LMS - #11806

Merged
saleem-latif merged 1 commit into
masterfrom
saleem-latif/WL-327
Mar 31, 2016
Merged

WL-327: Add SiteConfiguration and SiteConfigurationHistory models to LMS#11806
saleem-latif merged 1 commit into
masterfrom
saleem-latif/WL-327

Conversation

@saleem-latif

Copy link
Copy Markdown
Contributor

Hi @asadiqbal08 , @mattdrayer ,

Kindly review this PR, change set contains addition of SiteConfiguration model to theming app.

Description of Changes:
Add a new SiteConfiguration Model to theming app which would link to Django site in OneToOne relationship for persisting site specific configurations. This model can have a JSONField to keep configurations in JSON. To persist each change to this model we can add a separate Model SiteConfigurationHistory which should persist history of SiteConfiguration model. We can copy data from SiteConfiguration to SiteConfigurationHistory on post_save signal of SiteConfiguration model.

class SiteConfiguration(models.Model):
    site = models.OneToOneField(Site, related_name='configuration')
    values = JSONField(null=False, blank=True, load_kwargs={'object_pairs_hook': collections.OrderedDict})

class SiteConfigurationHistory(TimeStampedModel):
    site = models.ForeignKey(Site, related_name='configuration_histories')
    values = JSONField(null=False, blank=True, load_kwargs={'object_pairs_hook': collections.OrderedDict})



@receiver(post_save, sender=SiteConfiguration)
def update_site_configuration_history(sender, instance, **kwargs): # pylint: disable=unused-argument

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to include a set of tests to cover this workflow

@tobz

tobz commented Mar 15, 2016

Copy link
Copy Markdown
Contributor

Does it make sense to build on top of ConfigurationModel here? It would give you history for free, and you could write custom methods to handle JSON serialization/deserialization.

@saleem-latif
saleem-latif force-pushed the saleem-latif/WL-327 branch 2 times, most recently from 9bbaf17 to d54a4f6 Compare March 29, 2016 09:53
@saleem-latif

Copy link
Copy Markdown
Contributor Author

@tobz I looked at the ConfigurationModel model here and I do not think it would be good fit in this scenario.

@mattdrayer

Copy link
Copy Markdown
Contributor

I'm thinking that these models should live somewhere outside of theming -- I'd prefer to add it to edx-django-sites-extensions, but it would result in a conflict with the existing SiteConfiguration model present in Otto/ecommerce. If we keep these specific to edx-platform perhaps we add a new module to edx-platform/openedx/core/models?

@saleem-latif

Copy link
Copy Markdown
Contributor Author

@mattdrayer I have incorporated your feedback changes, kindly take a look

@mattdrayer

Copy link
Copy Markdown
Contributor

@douglashall, FYI

@mattdrayer

Copy link
Copy Markdown
Contributor

I wasn't able to comment directly on the __init__.py file because it contains no lines -- can you please add a docstring to this module explaining the purpose of the new app and some details about the models and how to use them?

Comment thread cms/envs/common.py
# Theming
'openedx.core.djangoapps.theming',

'openedx.core.djangoapps.site_configuration',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a leading comment

@mattdrayer

Copy link
Copy Markdown
Contributor

@saleem-latif overall I think this change set looks good -- we need to improve context/understanding through better comments and docstrings, but as far as the implementation goes we are on the right track.

@douglashall

Copy link
Copy Markdown
Contributor

👍 from me once @mattdrayer's comments are addressed.

@saleem-latif

Copy link
Copy Markdown
Contributor Author

@mattdrayer I have updated existing comments and added some new doc strings, kindly take a look

@mattdrayer

Copy link
Copy Markdown
Contributor

Looks great -- 👍

@saleem-latif
saleem-latif merged commit 7b549bc into master Mar 31, 2016
@saleem-latif
saleem-latif deleted the saleem-latif/WL-327 branch July 27, 2016 10:51
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.

4 participants