Fix Consultation first public at #2959

Merged
merged 1 commit into from Jan 17, 2017

Projects

None yet

3 participants

@andrewgarner
Contributor
andrewgarner commented Jan 12, 2017 edited

A consultation can be published before it is due to open so it is incorrect to say it was first published at the time it opened.

Trello: https://trello.com/c/aLeUBH7W

@andrewgarner andrewgarner referenced this pull request in alphagov/govuk-content-schemas Jan 12, 2017
Closed

Remove Government requirement for Consultations and Speeches #480

@@ -217,7 +217,7 @@ def assert_featured(doc)
end
view_test "#index orders consultations by first_published_at date by default" do
- consultations = 5.times.map {|i| create(:published_consultation, opening_at: (10 - i).days.ago) }
+ consultations = 5.times.map {|i| create(:published_consultation, first_published_at: (10 - i).days.ago) }
@fofr
fofr Jan 12, 2017 Member

Ordering consultations by published date rather than opening date is possibly an undesirable side effect.

@gpeng
gpeng Jan 16, 2017 Contributor

I think we should use opening_at for ordering now we are making a distinction between the two. That is kind of what we are doing now although we are calling that date first_published_at

@andrewgarner
andrewgarner Jan 16, 2017 Contributor

The UI allows you to refine the search by "Published after" and "Published before" which is exactly the behaviour this PR is now implementing correctly.

screen shot 2017-01-16 at 20 18 48

The change in the test setup is needed because the first_published_at value will otherwise get set on save unless it is overridden when using the factory.

I've looked into how the search ordering works and I think the only way to implement the behaviour you describe would be to override the first_public_at method from Edition which is normally:

def first_public_at
  first_published_at
end

That doesn't make sense to me as the value returned would be unexpected.

@@ -217,7 +217,7 @@ def assert_featured(doc)
end
view_test "#index orders consultations by first_published_at date by default" do
- consultations = 5.times.map {|i| create(:published_consultation, opening_at: (10 - i).days.ago) }
+ consultations = 5.times.map {|i| create(:published_consultation, first_published_at: (10 - i).days.ago) }
@gpeng
gpeng Jan 16, 2017 Contributor

I think we should use opening_at for ordering now we are making a distinction between the two. That is kind of what we are doing now although we are calling that date first_published_at

@gpeng gpeng dismissed their review Jan 17, 2017

Discussed offline. Using first_published_at more accurately reflects how the UI is labelled and coupled with the changes we have made in government-frontend should make things clearer.

@gpeng
gpeng approved these changes Jan 17, 2017 View changes
@andrewgarner andrewgarner Fix Consultation first public at
A consultation can be published before it is due to open so it is
incorrect to say it was first published at the time it opened.
158956f
@andrewgarner andrewgarner merged commit 9731cc7 into master Jan 17, 2017

2 checks passed

continuous-integration/jenkins/branch This commit looks good
Details
default Build #10357 succeeded on Jenkins
Details
@andrewgarner andrewgarner deleted the fix-consultation-first-public-at branch Jan 17, 2017
@andrewgarner andrewgarner added a commit that referenced this pull request Jan 18, 2017
@andrewgarner andrewgarner Fix published Consultation first_published_at
#2959 introduced changes to
fix the `first_public_at` value to be when the Consultation was
published not when it opened. It however failed to backfill the
existing Consultation data so that the correct values are in place so
that the inherited code from Edition behaves as expected.

Consultation previously set `make_public_at(date)` as a NOOP causing
all existing Consultations to have a nil `first_published_at` value
when they were published. This migration backfills the Consultations
to have the value that would have been returned had the code not been
removed.

We have to skip validations as they expect the date to be in the present.
78ed20d
@andrewgarner andrewgarner added a commit that referenced this pull request Jan 19, 2017
@andrewgarner andrewgarner Fix published Consultation first_published_at
#2959 introduced changes to
fix the `first_public_at` value to be when the Consultation was
published not when it opened. It however failed to backfill the
existing Consultation data so that the correct values are in place so
that the inherited code from Edition behaves as expected.

Consultation previously set `make_public_at(date)` as a NOOP causing
all existing Consultations to have a nil `first_published_at` value
when they were published. This migration backfills the Consultations
to have the value that would have been returned had the code not been
removed.

We have to skip validations as they expect the date to be in the present.
c419498
@andrewgarner andrewgarner added a commit that referenced this pull request Jan 19, 2017
@andrewgarner andrewgarner Fix published Consultation first_published_at
#2959 introduced changes to
fix the `first_public_at` value to be when the Consultation was
published not when it opened. It however failed to backfill the
existing Consultation data so that the correct values are in place so
that the inherited code from Edition behaves as expected.

Consultation previously set `make_public_at(date)` as a NOOP causing
all existing Consultations to have a nil `first_published_at` value
when they were published. This migration backfills the Consultations
to have the value that would have been returned had the code not been
removed.

We have to skip validations as they expect the date to be in the present.
01045d2
@andrewgarner andrewgarner added a commit that referenced this pull request Jan 19, 2017
@andrewgarner andrewgarner Fix published Consultation first_published_at
#2959 introduced changes to
fix the `first_public_at` value to be when the Consultation was
published not when it opened. It however failed to backfill the
existing Consultation data so that the correct values are in place so
that the inherited code from Edition behaves as expected.

Consultation previously set `make_public_at(date)` as a NOOP causing
all existing Consultations to have a nil `first_published_at` value
when they were published. This migration backfills the Consultations
to have the value that would have been returned had the code not been
removed.

We have to skip validations as they expect the date to be in the present.
7a2914e
@andrewgarner andrewgarner added a commit that referenced this pull request Jan 19, 2017
@andrewgarner andrewgarner Fix published Consultation first_published_at
#2959 introduced changes to
fix the `first_public_at` value to be when the Consultation was
published not when it opened. It however failed to backfill the
existing Consultation data so that the correct values are in place so
that the inherited code from Edition behaves as expected.

Consultation previously set `make_public_at(date)` as a NOOP causing
all existing Consultations to have a nil `first_published_at` value
when they were published. This migration backfills the Consultations
to have the value that would have been returned had the code not been
removed.

We have to skip validations as they expect the date to be in the present.
e854ab3
@andrewgarner andrewgarner added a commit that referenced this pull request Jan 19, 2017
@andrewgarner andrewgarner Fix published Consultation first_published_at
#2959 introduced changes to
fix the `first_public_at` value to be when the Consultation was
published not when it opened. It however failed to backfill the
existing Consultation data so that the correct values are in place so
that the inherited code from Edition behaves as expected.

Consultation previously set `make_public_at(date)` as a NOOP causing
all existing Consultations to have a nil `first_published_at` value
when they were published. This migration backfills the Consultations
to have the value that would have been returned had the code not been
removed.

We have to skip validations as they expect the date to be in the present.
6c9d152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment