Skip to content
This repository has been archived by the owner on May 30, 2019. It is now read-only.

Removed unique=True from UserOpenID.claimed_id #1

Closed
wants to merge 3 commits into from

Conversation

symbolist
Copy link
Contributor

https://openedx.atlassian.net/browse/TNL-3664

django-openid-auth has claimed_id = TextField(unique=True). However, TextFields cannot be unique in MySQL: django/django@e8cbc2b
https://docs.djangoproject.com/en/1.8/ref/models/fields/#textfield
https://code.djangoproject.com/ticket/2495

This results in the following error when running migrations:
"django.db.utils.OperationalError: (1170, "BLOB/TEXT column 'claimed_id' used in key specification without a key length")"

@symbolist
Copy link
Contributor Author

@macdiesel @nedbat I have imported the code from https://code.launchpad.net/django-openid-auth and made this change. Thoughts?

@symbolist
Copy link
Contributor Author

Btw, it does fix the issue. And as far as I understand in Django 1.4 unique=True on TextFields was ignored. For example on a sandbox with Django 1.4 I do not see a unique index on this field.

@macdiesel
Copy link
Member

👍

@symbolist
Copy link
Contributor Author

@nedbat @doctoryes This needs one more review.

@doctoryes
Copy link
Contributor

@symbolist Above, you say you "imported the code from launchpad and made this change." Does this mean you submitted the change upstream to the module itself? Other than this question, looks fine.

@symbolist
Copy link
Contributor Author

@doctoryes We are planning to change the field to CharField later so this is not done yet. Also this makes a change to an existing migration. I am not sure if upstream can accept it.

@doctoryes
Copy link
Contributor

👍

@doctoryes
Copy link
Contributor

Shouldn't this PR be against the 'master' branch instead of the 'upstream' branch? Typically, an 'upstream' branch would reflect the state of the upstream repo - while 'master' would reflect our edX changes.

@doctoryes
Copy link
Contributor

I'm going to hold off on merging this until @symbolist comments.

@symbolist
Copy link
Contributor Author

@doctoryes When I was working on this, master did not have the right commit history, so I pushed the upstream code into the upstream branch. That has been resolved (master has the correct history) so we can merge these changes into master instead.

@symbolist symbolist closed this Nov 24, 2015
@symbolist symbolist mentioned this pull request Nov 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants