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

Rename SectionEdition's database collection from specialist_document_editions to manual_section_editions #897

Conversation

floehopper
Copy link
Contributor

  • The new name more closely matches the model class name and removes one of the last vestiges of specialist documents in the app.

  • I've prefixed the new collection name with "manual" rather than just using "section_editions", because it turns out that this app is sharing a database with a number of other apps, e.g. Publisher, and I felt as if "section_editions" was too generic and might lead to confusion.

  • I've used methods on Mongoid::DB rather than ActiveRecord::ConnectionAdapters::SchemaStatements#rename_table, because of an incompatibility (see Fix incompatibility between mongoid_rails_migrations and mongoid versions #896) between mongoid_rails_migrations and the version of mongoid we are using which results in the following exception:

NoMethodError: undefined method `default_session' for Mongoid:Module
  • I've had to include code to drop the target collection if it exists and is empty before doing the rename, because it appears that something (presumably Mongoid) is creating the collection on application startup based on the existence of the SectionEdition class and its call to store_in with the new collection name. This means that an empty manual_section_editions collection is created before the migration is run and if we didn't drop the collection, the rename would fail. I'm only doing this if the collection is empty, on the off-chance that the collection really exists and contains some important data.

Closes #848.

@chrislo
Copy link
Contributor

chrislo commented Mar 21, 2017

@floehopper This looks good - I agree with the choice of collection name. It feels odd that this app shares a database with other apps but hopefully this will make it more obvious where this collection is managed from to anyone looking at that DB. Are you reasonably confident that none of the other apps rely on this collection being named specialist_document_editions? As-in other apps don't read or write from this collection?

Copy link
Contributor

@chrislo chrislo left a comment

Choose a reason for hiding this comment

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

My comment shouldn't stop you merging this!

@floehopper
Copy link
Contributor Author

@chrislo:

Thanks for reviewing.

It feels odd that this app shares a database with other apps but hopefully this will make it more obvious where this collection is managed from to anyone looking at that DB.

I've created #893 to investigate using a separate DB.

Are you reasonably confident that none of the other apps rely on this collection being named specialist_document_editions? As-in other apps don't read or write from this collection?

Good point. I'm fairly confident. Historically I think govuk_content_api would have used it via govuk_content_models. However, when Specialist Documents began to be published via the Publishing API, this will've become unnecessary. The following PRs are relevant:

So all in all, I'm happy to go ahead and merge this.

* The new name more closely matches the model class name and removes
one of the last vestiges of specialist documents in the app.

* I've prefixed the new collection name with "manual" rather than just
using "section_editions", because it turns out that this app is sharing
a database with a number of other apps, e.g. Publisher, and I felt as
if "section_editions" was too generic and might lead to confusion.

* I've used methods on `Mongoid::DB` rather than
`ActiveRecord::ConnectionAdapters::SchemaStatements#rename_table`,
because of an incompatibility (see #896) between
`mongoid_rails_migrations` and the version of `mongoid` we are using
which results in the following exception:

    NoMethodError: undefined method `default_session' for Mongoid:Module

* I've had to include code to drop the target collection if it exists
and is empty before doing the rename, because it appears that something
(presumably Mongoid) is creating the collection on application startup
based on the existence of the `SectionEdition` class and its call to
`store_in` with the new collection name. This means that an empty
`manual_section_editions` collection is created before the migration is
run and if we didn't drop the collection, the rename would fail. I'm
only doing this if the collection is empty, on the off-chance that the
collection really exists and contains some important data.

Closes #848.
@floehopper
Copy link
Contributor Author

I've just noticed that alphagov/govuk_content_models#324 did not remove manuals and that there is still a RenderedManual model in that repo. However, the fact that it's a RenderedManual and not a Manual and is presumably mapped to a rendered_manuals collection suggests that it is not relying on the manual_records collection provided by Manuals Publisher.

Looking at my copy of the production mongo db, it appears that both rendered_specialist_documents and rendered_manuals collections exist. My guess is that they are no longer used, but I'm going to ask in Slack to confirm.

@floehopper
Copy link
Contributor Author

As far as I can see, manuals-frontend only uses the content store and not the content api and so I can't see how any other apps can be depending on the specialist_document_editions collection name. I plan to merge this PR in the morning.

@floehopper floehopper force-pushed the rename-specialist-document-editions-collection-to-manual-section-editions branch from e14e57a to feb2d64 Compare March 21, 2017 21:30
@floehopper floehopper merged commit 5901f70 into master Mar 22, 2017
@floehopper floehopper deleted the rename-specialist-document-editions-collection-to-manual-section-editions branch March 22, 2017 09:18
@chrislo chrislo modified the milestone: Legacy Apr 25, 2017
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.

Rename the specialist_document_editions Mongo collection
2 participants