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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@cpennington
Member

cpennington commented Mar 10, 2015

@ormsbee @e0d @doctoryes: Review?

@wedaly: LinkedInAddToProfileConfiguration was missing a migration on master.

@wedaly

View changes

...nt/migrations/0047_auto__add_field_linkedinaddtoprofileconfiguration_company_identifier__.py Outdated
# 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)

This comment has been minimized.

@wedaly

wedaly Mar 10, 2015

Contributor

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?

This comment has been minimized.

@cpennington

cpennington Mar 10, 2015

Member

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

This comment has been minimized.

@cpennington

cpennington Mar 10, 2015

Member

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

@@ -179,6 +179,17 @@ class UserStanding(models.Model):
standing_last_changed_at = models.DateTimeField(auto_now=True)
class UserStandingConfig(ConfigurationModel):
disabled = models.TextField(
help_text="White-space separated list of usernames of users who should be disabled",

This comment has been minimized.

@doctoryes

doctoryes Mar 10, 2015

Member

How many usernames do we expect ended up in this list? I realize we probably have no idea. But it would only take 10s of usernames in this list before it became unmanageable to a non-coder. Is there a way to perhaps alphabetize it on an addition?

This comment has been minimized.

@cpennington

cpennington Mar 10, 2015

Member

Addition is only going to be happening from the admin interface. But yes, as this grows to longer than a small handful, I agree, this interface isn't going to be great. I'm honestly not sure how to best balance the needs of not spamming the cache (by creating an entry for every user) vs not having a single giant list. I'm inclined for the moment to simply lean towards what's simple for the small list we have, and tackle the large list later.

This comment has been minimized.

@doctoryes

doctoryes Mar 10, 2015

Member

I'm fine with that. But - perhaps a comment of warning for future civilizations is appropriate? Like:

This was done to fix [a performance problem] and is designed for X users. Likely problems with more users than X:

  • Y
  • Z

This comment has been minimized.

@ormsbee

ormsbee Mar 10, 2015

Member

I wouldn't worry about cache spamming. I think we're running with something like 4-6GB of cache on prod. Having a simple boolean entry for every user in the system barely dents that.

),
)
return HttpResponseForbidden(msg)
if user.username in UserStandingConfig.current().disabled_list:

This comment has been minimized.

@doctoryes

doctoryes Mar 10, 2015

Member

Again, fine for 10s of users - not great for 1000s of users. The whitespace-separated list will break down long before this search becomes too costly. If we really cared now, a SortedList could be used.

@adampalay

This comment has been minimized.

Contributor

adampalay commented Mar 23, 2015

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

This comment has been minimized.

Member

cpennington commented Mar 23, 2015

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