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

create a new table to store featured projects #19510

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Dec 5, 2017

Cleaner follow up to #19500.

Overall, we want to allow those with LevelBuilder permissions to designate exemplary projects such that they can be highlighted on the public gallery. This PR sets up a table to store project ids for the projects we would like to feature. Work to allow the designation of projects is currently in progress in #19499, and follow up work will use the project ids stored in this table to pull projects from the storage apps table to be displayed in the gallery.

Copy link
Contributor

@aoby aoby left a comment

Choose a reason for hiding this comment

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

Run the migration in your branch. This PR should also contain the updated schema in schema.rb, and annotations on the new FeaturedProject model.

t.integer :who_featured_user_id
t.integer :whose_project_user_id

t.timestamps
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we decide we only wanted created_at not updated_at? If so, instead of t.timestamps, create the desired column directly:

t.datetime :created_at

create_table :featured_projects do |t|
t.integer :storage_app_id
t.integer :who_featured_user_id
t.integer :whose_project_user_id
Copy link
Member

Choose a reason for hiding this comment

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

this column is denormalized, because you could get it instead by joining with the storage_apps table to get the storage_id, and then joining that with the user_storage_ids table to get the user id. denormalization is typically done only as a performance optimization.

My initial inclination was that it would be ok to denormalize in this situation. However after pulling in @jeremydstone , Jeremy's recommendation is to avoid denormalizing as it is an anti-pattern because it represents duplicate state. This could be a problem for example if we were to implement transferring ownership of projects from one user to another. There may also already be an existing codepath where this happens: when the signed-out owner of a project signs up for an account, causing the project owner user id to go from NULL to an actual value.

So, my recommendation (based on Jeremy's recommendation) would be to remove this column, and to join with the storage_apps and user_storage_ids tables instead.

I can help you figure out the syntax for this. Here is one example where we join to find the user id associated with a project owner:

projects[project_group] = PEGASUS_DB[:storage_apps].
select(*project_and_user_fields).
join(:user_storage_ids, id: :storage_id).
join(users, id: :user_id).
where(state: 'active', project_type: project_types, abuse_score: 0).
where {published_before.nil? || published_at < DateTime.parse(published_before)}.
exclude(published_at: nil).
order(Sequel.desc(:published_at)).
limit(limit).
map {|project_and_user| get_published_project_and_user_data project_and_user}.compact

Copy link
Contributor

Choose a reason for hiding this comment

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

Shame the storage apps aren't in Rails, or we'd be able to use a handy ActiveRecord association. +1 for not denormalizing prematurely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will joining with the storage_apps and user_storage_ids tables also handle the issue of updating the featured projects list if a project is deleted from storage_apps? We'd end up in a sticky situation if the featured project list was referencing a project that had since been deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean "updating the featured projects list"? I'm pretty sure (test this) that Dave's query will return nil if the storage app, or any of its joined fields no longer exist. We'll have to handle that case appropriately. What do we want to happen if a featured project no longer exists?

Copy link
Member

Choose a reason for hiding this comment

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

when projects in storage_apps are deleted, the state column is changed from "active" to "deleted".

What do you mean "updating the featured projects list"?

from a DB perspective, the featured projects list is just the result of our query on this table, joined with the storage_apps and user_storage_ids table.

What do we want to happen if a featured project no longer exists?

We'll want to exclude it from the featured projects list.

Will joining with the storage_apps and user_storage_ids tables also handle the issue of updating the featured projects list if a project is deleted from storage_apps?

Yes, the where(state: 'active' portion of the query I linked to ensures that deleted projects are not returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a project is deleted from storage_apps and that project is on the featured project list, I want it to also be deleted from the featured project list.

@Erin007
Copy link
Contributor Author

Erin007 commented Dec 6, 2017

Okay, I think this PR is ready for another look. I removed whose_project_user_id and updated_at and ran the migration so the change to the schema is reflected. This should put us in a good position to join with the storage_apps table when we retrieve the projects to display in the gallery. Thank you for the helpful feedback, @aoby and @davidsbailey!

@Erin007 Erin007 merged commit eafee90 into staging-next Dec 6, 2017
@Erin007 Erin007 deleted the featured-project-table-tidy branch December 6, 2017 22:44
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

LGTM

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

3 participants