Rename specialist document #837
Conversation
chrislo
force-pushed
the
rename-specialist-document
branch
2 times, most recently
from
ec9dbc1
to
d8d4ac7
Compare
I took a recent dump of production data and checked out this branch locally. I clicked around the manuals-publisher UI and can confirm that everything appears to work as expected. |
|
Thanks @chrisroos - I've rebased this against master so I think it's ready for review. |
|
I've added issue #848 to prompt us to rename the Mongo collection once this has been merged. |
| documents = subject.documents.to_a | ||
| expect(documents.size).to eq 2 | ||
|
|
||
| document_1 = documents[0] | ||
| expect(document_1).to be_a ::SpecialistDocument | ||
| expect(document_1).to be_a ::Section |
There was a problem hiding this comment.
Do you know why the :: prefix is needed here and elsewhere in this file?
There was a problem hiding this comment.
No, it struck us as odd too - will investigate after merging.
| @@ -6,7 +6,7 @@ | |||
| require "builders/manual_document_builder" | |||
| require "manual_with_documents" | |||
| require "slug_generator" | |||
| require "specialist_document" | |||
| require "section" | |||
There was a problem hiding this comment.
Is this require statement still necessary now that the classname matches the filename?
There was a problem hiding this comment.
Possibly not - I think we'll take a look at this after we merge this PR, there's probably a few unnecessary requires scattered around.
| @@ -7,7 +7,7 @@ class Attachment | |||
| field :file_id, type: String | |||
| field :file_url, type: String | |||
|
|
|||
| embedded_in :specialist_document_edition | |||
| embedded_in :section_edition | |||
There was a problem hiding this comment.
I think it would be useful to mention why this change does not require any database migration, i.e. can you confirm that none of the existing attachments store a reference to their parent edition using this key. I'm guessing it's because this is an embedded document...?
There was a problem hiding this comment.
Yep, that's correct. I've addressed this comment in an improved commit message on b0b409a.
| @@ -1,13 +1,12 @@ | |||
| require "specialist_document_repository" | |||
| require "manual_repository" | |||
| require "specialist_document_edition" | |||
| require "section_edition" | |||
There was a problem hiding this comment.
Is this require statement necessary now that the class name matches the filename?
|
There are a bunch of classes named |
|
I'm going to rebase this on master given all the recent changes. |
We want to rename this class. To make it easier to do this we set the mongo collection that the data is stored in explicitly. This means we can change the code independently from the persistence.
chrisroos
force-pushed
the
rename-specialist-document
branch
from
d8d4ac7
to
ef6f394
Compare
|
I've rebased and force pushed. I'm going to hold off merging until I can pair with @chrislo to address @floehopper's comments and to double check that we're happy with the name |
Although we change the name of the collection that attachments are
embedded in (with `embedded_in :section_edition` in
`app/models/attachment.rb`) we don't need to migrate any data
here. The combination of `embedded_in` on attachments and
`embeds_many` on section editions means that a record in the
underlying mongodb section edition collection looks like:
{ _id: 'abcd1234',
... some other fields ...,
attachments: [
{_id: 'efgh5678',
filename: 'foo.txt'
... some other fields ...
}
]
}
That is, the relationship is defined by embedding Attachment records
in an `attachments` array under whatever is defined as the parent
collection. This continues to work when we rename the parent
collection.
We currently have no intention to make this refactor. TODO items belong elsewhere.
This context has no associated expectations so can be removed.
We are removing references to `specialist_document` and replacing them with `section`. Inlining the call to `Section.new` seems more intention-revealing than renaming the method.
This factory does not appear to be used so can be removed.
We noticed here that `manual` is stubbed to receive a `documents` method. In practice ManualUpdateType receives a ManualWithDocuments instance and we should probably look to rename that to ManualWithSections and give it a `sections` method instead.
chrislo
force-pushed
the
rename-specialist-document
branch
from
ef6f394
to
b0aa02e
Compare
|
I've raised #867 to make sure we keep track of the suggestion to rename |
|
Rebasing against master and force pushing before merging. |
This PR replaces the use of the term "specialist document" with the term "section". The publishing of what are known as Specialist Documents was moved to a separate application some time ago. However the name still remains in this codebase and seems to be used to refer to a section of a manual. We've gone with
Sectionas opposed to something likeManualSectionas the application currently only has responsibility for manuals and their sections and this seems to match the language used in the UI.This PR does not currently touch any data - the underlying mongodb collection for
SectionEditionis stillspecialist_document_editions. I'd like someone to help check that this continues to work with a recent copy of a production-like database.I think we'll rename the collection in a subsequent commit after this one has landed as we need to do some investigation about whether that would involve application downtime.