ponder-lab / Imperative-DL-Study-Web-App

0 stars 0 forks source link

Categorizers are missing a foreign key into Django users #147

Closed khatchad closed 2 years ago

khatchad commented 2 years ago

Found by @ZhongweiL

Currently, Categorizers have a char field that represents the Django user for which they are related. There are no constraints on the values of this field from what we can see in the Categorizer class. This is potentially very dangerous since the field can have any arbitrary char value. Instead, this field should represent a foreign key into Django users. In fact, it should be a 1-1 relationship.

For deleting, I would say that we should keep the categorizer around in case the Django user is deleted since we still need to know about their categorizations. Likewise, we want to keep the Django user around in the case that the Categorizer is deleted because they have transitioned into a different role. As such, we should use models.DO_NOTHING for the on_delete parameter as in https://github.com/ponder-lab/Imperative-DL-Study-Web-App/blob/9531da86d25858ecaee7232177e4281700be548b/ponder/models.py#L96.

To fix this problem, we need to add something like the following (from the docs):

user = models.OneToOneField(
        User,
        on_delete=models.DO_NOTHING,
        primary_key=True,
    )

Where User is the model for Django Users. @mZneit, can you please confirm that this is correct? Also, @ZhongweiL, you don't need to import this model class because we see it here: https://github.com/ponder-lab/Imperative-DL-Study-Web-App/blob/9531da86d25858ecaee7232177e4281700be548b/ponder/models.py#L2.

The only thing I am unsure about is whether you can do this with a model that is outside of our application. @mZneit, is this possible?

https://github.com/ponder-lab/Imperative-DL-Study-Web-App/blob/9531da86d25858ecaee7232177e4281700be548b/ponder/models.py#L63

khatchad commented 2 years ago

Test A:

  1. Make a new Django user that is in the categorizer group.
  2. Login to the app.
  3. Are you prompted to create a categorizer object?
  4. If yes, can you now do categorizations afterward?
  5. If no, then we have a bug.

Test B:

  1. Make a new Django user that is not in the categorizer group.
  2. Login to the app.
  3. Are you prompted to create a categorizer object?
  4. If yes, you have a bug.
  5. If no, then that's correct.
khatchad commented 2 years ago

@ZhongweiL Please make this change and redo the tests that you did as part of https://github.com/ponder-lab/Imperative-DL-Study-Web-App/issues/132#issuecomment-964599800 and https://github.com/ponder-lab/Imperative-DL-Study-Web-App/pull/143#issue-1052248975.

ZhongweiL commented 2 years ago

1637170520(1) I'm getting an error when I try to migrate the change.

khatchad commented 2 years ago

@ZhongweiL Can you just paste the text instead of the screenshot? It's a little blurry.

khatchad commented 2 years ago

Also, please open a PR so we can see what the code change looks like.

khatchad commented 2 years ago

Reference this issue in the PR, please. @mZneit and I guess that you are referencing the wrong fields. Where is user_id from, and where is username (which tables)?

khatchad commented 2 years ago

Also, please make a test for this in the CI.

khatchad commented 2 years ago

@ZhongweiL Work on model tests and form tests for this change.

khatchad commented 2 years ago

I kicked out the changes for this issue in 48fd17895a21ef59509da55392efd09170e4f4f0 and c104e7765953a373fe10139b4bb9c52f0e84337a. They don't work.

khatchad commented 2 years ago

See https://github.com/ponder-lab/Imperative-DL-Study-Web-App/issues/158#issuecomment-993881594.

ZhongweiL commented 2 years ago

@khatchad How about setting the user field as unique? It also makes sure that each user can only have one entry in the categorizers table and we can also reference the categorizer with a string.

ZhongweiL commented 2 years ago

@khatchad The problem is with the string representation of Categorizer defined in __str__ function. Currently, it returns self.user, which references to a user object after the changes we make. To solve that, we can convert the user to its string representation (the username) before it gets returned.

ZhongweiL commented 2 years ago

Either solution would work.

khatchad commented 2 years ago

Failing https://github.com/ponder-lab/Imperative-DL-Study-Web-App/pull/165/checks.