-
Notifications
You must be signed in to change notification settings - Fork 16
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
Switch fully to documents and editions #683
Conversation
7c5d58f
to
8e3366d
Compare
Looks good Tom - think you might want to separate that migration out though as the update and inserts will be slow queries. Probably want to populate this slowly in the same way we've done the pillars. |
8556833
to
2207ad6
Compare
c2bbf82
to
c0b21f5
Compare
ec1dfce
to
eeafeec
Compare
43f7c3e
to
0e88327
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paired on reviewing this with @boffbowsh , as a mentoring exercise. I'm off for Xmas now but he says he's happy to answer any questions!
@@ -50,7 +50,6 @@ | |||
end | |||
|
|||
create_table "content_items", force: :cascade do |t| | |||
t.string "content_id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of these lines in the schema seems to be the consequence of another migration - could you check the source of these lines?
end | ||
|
||
def down | ||
remove_index :documents, [:content_id, :locale] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the index isn't necessary if you're dropping the documents table.
Also, instead of up
and down
, you could have a single change
method.
)' | ||
|
||
remove_column :content_items, :content_id | ||
remove_column :content_items, :locale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code that uses these columns needs to be changed to get the content_id and locale from documents before we can remove the columns.
def up | ||
create_table :documents do |t| | ||
t.string :content_id | ||
t.string :locale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these should be not null.
@@ -102,7 +102,9 @@ def select_fields(scope) | |||
when :base_path | |||
"content_items.base_path as base_path" | |||
when :locale | |||
"content_items.locale as locale" | |||
"documents_content_items.locale as locale" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is documents_content_items
coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the name that Rails gave to the join of documents in this case, because there was a subquery with a join on the name documents
already. I agree it's unclear though and I don't really like it, will try and find a way to make it clearer.
@@ -3,7 +3,8 @@ module GetContent | |||
def self.call(content_id, locale = nil, version: nil, include_warnings: false) | |||
locale_to_use = locale || ContentItem::DEFAULT_LOCALE | |||
|
|||
content_items = ContentItem.where(content_id: content_id, locale: locale_to_use) | |||
content_items = ContentItem.joins(:document). | |||
where('documents.content_id': content_id, 'documents.locale': locale_to_use) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also use:
where(documents: {content_id: content_id, locale: locale_to_use})
@@ -59,7 +59,7 @@ def content_items | |||
scope = ContentItem.where(document_type: lookup_document_types) | |||
scope = scope.where(publishing_app: publishing_app) if publishing_app | |||
scope = scope.where(state: states) if states.present? | |||
scope = scope.where(locale: locale) unless locale == "all" | |||
scope = scope.joins(:document).where('documents.locale': locale) unless locale == "all" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above
8f180a9
to
76134ea
Compare
c1a117a
to
211648c
Compare
@@ -62,28 +62,24 @@ def increment_live_lock_version | |||
lock_version.save! | |||
end | |||
|
|||
def document | |||
@document ||= Queries::GetDocument.(content_id, locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can access these values straight from payload so hopefully we can kill of the need for individual methods for them?
def document | ||
@document ||= Queries::GetDocument.(content_id, locale) | ||
end | ||
|
||
def draft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we kill this method and just use document.draft
?
locale: locale, | ||
state: "draft", | ||
) | ||
@draft ||= document.draft | ||
end | ||
|
||
def live |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we kill this method and just use document.live
?
def pessimistic_content_item_scope | ||
ContentItem.where(content_id: content_id, locale: locale).lock | ||
def document | ||
@document ||= Queries::GetDocument.(content_id, locale) | ||
end | ||
|
||
def previously_published_item | ||
@previously_published_item ||= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use document.live
here?
state: "draft", | ||
content_store: "draft", | ||
user_facing_version: content_item.user_facing_version, | ||
) | ||
) | ||
|
||
# update these fields to also update the underlying document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need marking as temporary code?
@@ -96,6 +94,29 @@ class ContentItem < ApplicationRecord | |||
end | |||
end | |||
|
|||
def as_json(options = {}) | |||
super(options).merge(content_id: content_id, locale: locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these lookups be prefixed with document
@@ -112,16 +133,14 @@ def draft_cannot_be_behind_live | |||
if state == "draft" | |||
draft_version = user_facing_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify this to be document.live.user_facing_version
?
message: 'must be a supported locale' | ||
} | ||
|
||
def draft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably improve this somehow, maybe using a scope or similar. Do you think we should memoize here? I guess we hit it a lot, probably should act like a relation (I've been considering it one in my dozy just back from New Year state heh)
@@ -0,0 +1,25 @@ | |||
module Queries | |||
module GetDocument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be named to indicate the lock like Queries::LockedDocument
?
Document.find_or_create_by!( | ||
content_id: content_id, | ||
locale: locale_to_use | ||
).lock! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon we can refactor this to not have a second query for the lock. Will have a think.
c379d3c
to
84e1b5f
Compare
9c65e73
to
7dc077e
Compare
Now called import_content_id_events which is more descriptive of what the task actually does.
This data will have been "cleaned up" many moons ago by now and this task is no longer needed.
As part of the split of ContentItems into Documents and Editions.
These monkey patches are used so that we can continue using a polymorphic association on LockVersion to ContentItem/Edition without changing what is saved in the database currently - namely a `target_type` of "ContentItem". This allows us to swap from this branch to the currently released version without pain. This patching should live for a very short period of time as we will not need to maintain backwards compatibility once this is deployed and we also intend to drop the lock_version table.
These are changes further to the initial rebasing of #729 which make better use of document.
Since we can and do validate UUID's with a regex it makes sense for us to use the correct format for these as a route constraint.
Changed to no longer refer to content_item, also made more succinct.
These clear up the cases where the factory is on the next line so they are all consistently on the same line as the method opening.
Although valid syntax in Ruby this is inconsistent with our usage of methods.
As ActiveRecord handles the memoization for us it seems redundant to have two levels of this in.
This code would have accidentally been concealing errors.
This replaces the many places where we run `joins(:document)`.
A small refactoring to try make a particularly difficult line of code to follow easier to understand.
I think this is easier to follow than the concatentation approach it replaces, still not super easy but seemed less heavy duty than squigly heredoc.
This makes the matcher more specific. It also removes the strings that are output on a failure. We don't use them in other places and in this instance they don't seem particularly useful.
For usage with Documents and Editions.
This is no longer used.
13fa6fe
to
159a59f
Compare
Thanks @dougdroper - Just going to run this on integration for an hour before merging. I'll see if I can get a deploy slot for today. |
This method was renamed in the past (#683), but the relevant documentation still has the old method name.
This method was renamed in the past (#683), but the relevant documentation still has the old method name. Co-authored-by: Cristina <[email protected]>
This method was renamed in the past (#683), but the relevant documentation still has the old method name. Co-authored-by: Cristina <[email protected]>
This method was renamed in the past (#683), but the relevant documentation still has the old method name. Co-authored-by: Cristina <[email protected]>
Second part of the documents and editions experience. Here we remove the
locale
andcontent_id
columns from content items and switch to using only documents as the canonical source of data.There are no migrations so this should be easy to deploy after #705.