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

Move user standing to configmodel #7283

Conversation

cpennington
Copy link
Contributor

@ormsbee @e0d @doctoryes: Review?

@wedaly: LinkedInAddToProfileConfiguration was missing a migration on master.

# Adding field 'LinkedInAddToProfileConfiguration.trk_partner_name'
db.add_column('student_linkedinaddtoprofileconfiguration', 'trk_partner_name',
self.gf('django.db.models.fields.CharField')(default='', max_length=10, blank=True),
keep_default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm really surprised to see this. It looks like these fields are already being added by:

https://github.com/edx/edx-platform/blob/master/common/djangoapps/student/migrations/0044_linkedin_add_company_identifier.py

https://github.com/edx/edx-platform/blob/master/common/djangoapps/student/migrations/0045_add_trk_partner_to_linkedin_config.py

There were a couple of non-standard things that happened with this migrations that might be causing this. First, I initially deleted the trk_partner_name, which wasn't backwards compatible, so I replaced that migration with the current version of 0044. Also, a separate migration from the solutions team got merged as 0044, then renamed to 0046. Would either of those changes explain why South is trying to add these fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. If the solutions migration didn't include your fields in their models dictionary, South wouldn't know that they'd already been migrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although, the definition of that field in-fact doesn't include a default value, and is not null.

@adampalay
Copy link
Contributor

I'm not really sure why we want to convert UserStanding to a newline-separated list, for the reason @doctoryes outlined about its maintainability. Why don't we just add the same caching to the UserStanding model and make a django admin interface for it for its UI?

@cpennington
Copy link
Contributor Author

Because I'd rather use a common setup for caching/django admin than have to sprinkle it all over the codebase.

I was in the process of extending ConfigurationModel to allow additional keys (besides the time), at which point we can use the user as a key.

@cpennington cpennington closed this May 3, 2016
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.

None yet

5 participants