broadinstitute / genetic-prevalence-estimator

https://genie.broadinstitute.org/
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Add public lists #156

Closed rileyhgrant closed 9 months ago

rileyhgrant commented 12 months ago

Part 1/2 of: #145

Adds the ability to make lists public in GeniE

Currently, to view a variant list a user must be signed in, and have access permissions to the particular variant list (either as a viewer, editor, or owner).

With this PR, esers who are owners of a variant list can submit a list for approval from Staff members. Staff members can see a list of all variant lists that have been submitted for public approval, and their current status. If a staff member approves a list, any user (including users who have not logged into the app and inactive users) can see the public lists page and navigate to any approved public list's corresponding variant list page.

rileyhgrant commented 10 months ago

Hiya @phildarnowsky-broad, I've addressed most of the comments that we talked about when doing a code walk the other day, save for one which I will touch on below, and am now formally requesting review from you, at your convenience. Thanks!


One comment I'm still on the fence about was in regards to renaming some of the views to be more indicative of their usage, namely:

When going back through the PR, I realized I mischaractarized the roles of these serializers.

For those reasons, I don't think the names we discussed strictly apply, though I like the idea of making it very clear that a particular view has more privileged permissions for admins, I feel like this may not be the use case. Of course, let me know your opinion differs and we can revisit it. I just wanted to mention it here in a comment since it was not changed in the last commit, even though we discussed it.

rileyhgrant commented 10 months ago

If the data model is as I think I might recommend some changes there, though I'm rusty on relational DB design so I would have to think more about it. Do I understand how this works correctly?

Yup, that is the current design, except just to make sure I'm on the same page on the last point:

And then a variant list with null public_status would be a private list?

A null public_status as in, there is no entry in the PublicVariantList table for a given Variant List, yes.

rileyhgrant commented 10 months ago

Hiya @phildarnowsky-broad, I've re-worked the data model as discussed to remove the seperate table that was tightly coupled, instead having the data live on the Variant List table.

One thing I wanted to mention, since we had talked about it in some detail, is the lack of database level Check constraints. There are currently none because:

  1. Check constraints, as far as I could tell, cannot be enforced for foreign key relationships
  2. Metadata that was specific to the separate Public Variant List table is now shared with the Variant List table

For these two reasons, the previously discussed model level constraints regarding the fields specific to public variant lists are not present.

The fields pertinent to whether or not a list is public have been reduced to the public_status, and a user who updated the status (public_status_updated_by, a foreign key relationship). The Variant list table already keeps track of when it was last updated, and it is logical to me that a Variant list that is now private could very well have been updated by someone (who chose to make it private again), and is not necessarily null.

Of course, I'm happy to discuss more. I am now formally re-requesting your review. Thanks!