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

Derive Government from Edition at run-time, rather than persisting on Document #2008

Merged
merged 7 commits into from Mar 4, 2015

Conversation

Projects
None yet
2 participants
@dsingleton
Contributor

dsingleton commented Mar 4, 2015

Taking this approach simplifies the code significantly, keeping the relationship to Government encapsulated in Edition, and there's not a need to de-normalise onto Document yet, and if becomes needed it can be added and backfilled then.

Changes:

  • Gets the right government for Speech, using the delivered_on in preference to first_published_at, as there are many examples where delivered_on is much earlier.
  • Government is now exposed by Edition#government, and derived at run-time, rather than being set during publishing.
  • Removes the Document.government field - which isn't used anywhere yet.
  • Adds factories for tests creating Governments and refactors the tests to be more readable.

This supports:

Paired with @tekin

dsingleton and others added some commits Mar 3, 2015

Set correct Government for Speech & Consultation
Government was set based on `first_published_at`, which does not work for consultations and speeches;

 - Consultations do not set a `first_published_at`
 - Speeches have a `delivered_on`, which may be different from
   `first_published_at`, and is usually earlier.

Add a new method to expose the right date to identify a Government responsible for the Edition.
David Singleton
Remove unused association on Government
This association is not used anywhere in the app. Getting rid of it
as a pre step to refactoring the derivation of government on editions.
Edition knows what Government it was created under
We've moving away from Government being set on Document, and 
instead being derived from the Edition itself.

Move the "which government owns this edition" logic into the Edition model itself. Update the publishing workflow to use this, rather than having to know about Edition/Government relationships.
Stop persisting Government on Document
We now derive Government from an Edition, on demand, rather than pre-calculating it and storing it on Document.
David Singleton
Refactor Government tests for clarity
- Break date related tests into their own class
- Use new government factories
- Generally much easier to read and understand intent 👍
David Singleton
Minor refactor to Government scopes
Don't use :scope when returning a single value, use class methods
instead.

Remove redundant nil (implicit from return) and make the guard vs
lookup more distinct
@edds

This comment has been minimized.

Show comment
Hide comment
@edds

edds Mar 4, 2015

Contributor

👍 Good changes.

Contributor

edds commented Mar 4, 2015

👍 Good changes.

dsingleton added a commit that referenced this pull request Mar 4, 2015

Merge pull request #2008 from alphagov/government-on-edition
Derive Government from Edition at run-time, rather than persisting on Document

@dsingleton dsingleton merged commit 99a80de into master Mar 4, 2015

1 check passed

default Build #5299 succeeded on Jenkins
Details

@dsingleton dsingleton deleted the government-on-edition branch Mar 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment