Looking at other similar step definitions, it looks as if this call to check_manual_is_drafted_and_published_with_no_public_timestamps should be a call to check_section_is_drafted_and_published_with_no_public_timestamps.

There seem to be other related inconsistencies in the related ManualHelpers methods.

This app has a dependancy on govuk_frontend_toolkit which it probably shouldnt have as doesn't and shouldnt have the GOV.UK look and feel as it is an admin app. It is currently only using things like %contain-floats from the govuk_frontend_toolkit so we should remove this dependancy.

We should decide whether or not it's worth keeping these scripts. If we do decide to keep them, we'll need to check they work and ensure that they remain working by adding the appropriate test coverage.

From this comment by @h-lame:

delete_documents_marked_for_delete, delete_duplicate_drafts and find_duplicate_documents are to help working around the fact that sometimes normal use of the app can result in duplicate manuals or sections being created. delete_documents_marked_for_delete is used when the users flag the duplicates so they are easy to remove (prefix the title with an xx I think), and the others try to find things programmatically. Getting to the bottom of the actual problem should mean we can get rid of all three of these (particularly if we also provide delete/withdraw functionality). See https://trello.com/c/R3kQxY9Z/74-duplication-bug-in-manuals for some thoughts on what the cause might be

As per @h-lame's comment on PR #798.

NOTE. This should ideally wait until PR #808 has been merged.

A pattern that other gov.uk apps have been using is a Services module that has class methods on it for each gds-api used by the app. Might be worth pushing this as far as that too. For example: https://github.com/alphagov/policy-publisher/blob/0f2fa349a043ae7acea37e22bbb08b57154fb30e/lib/services.rb

Unlike the other manual repositories, VersionedManualRepository looks up the relevant ManualRecord directly, i.e. not via an authorization scope.

I suspect this is OK, because this repository is only appears to be used in scripts, but it seems a bit odd.

Conversation from Slack govuk-developers channel:

@floehopper:

It looks as if contentapi, manuals_publisher, publisher, specialist_publisher and travel_advice_publisher apps all share the same mongo database.

Have I understood this correctly? And if so, is there any reason not to use separate databases for each app? Or do some of the apps "communicate" via the database?

@danielroseman:

content-api and publisher (and panopticon, now dead) did indeed communicate via the database. But content-api is being killed.

And travel-advice-publisher is being split out into its own db any day now

specialist-publisher only stores User information in its database, although I did think it had its own. Everything else is stored directly in the publishing-api.

@floehopper:

ok - that’s useful to know - it sounds as if (at least in the short term) it might make sense to use a separate database for manuals_publisher too

@danielroseman:

yes, talk to @thomasleese about how he's splitting out TAP.

In general it seems like a bad idea to share a database with others applications which presumably have write access.

See also #839.

We should decide whether or not it's worth keeping this script. If we do decide to keep it, we'll need to check that it works and ensure that it remains working by adding the appropriate test coverage.

From this comment by @h-lame:

republish_withdrawn_document - also works around functionality missing in the UI, although it's functionality that would be more complex to resurrect - the UI doesn't currently present the user with a list of sections that have been removed from the document, so it's not a case of putting a "republish" button on the UI. We may also have to do more work to move the document out of Manual#removed_documents and back into Manual#documents. Suspect this could be got rid of and we re-write as/when requested via a support ticket.

From this comment by @h-lame:

A good thing to do would be to turn these into rake tasks as then they can be run through jenkins and we'd get history of how often they're used. (See: https://github.com/alphagov/govuk-puppet/blob/f1c27d74dd0f361159bb5531473343730c692ea2/modules/govuk_jenkins/templates/jobs/run_rake_task.yaml.erb)

Steps to reproduce:

• Create a new manual and fill in the title, summary and body
• Click 'Save as draft' really fast lots of times

Expected results:

• There should only be one copy of the new manual in the manuals list

Actual results:

• There are lots of copies of the same manual in the manuals list

We should decide whether or not it's worth keeping these scripts. If we do decide to keep them, we'll need to ensure they work and ensure that they remain working by adding the appropriate test coverage.

From this comment by @h-lame:

delete_draft_manual and withdraw_manual work around functionality missing in the UI - you can't get rid of manuals via the UI even though the functionality exists in the system (we might need to check that these actually do the right thing wrt publishing-api). We can probably get rid of these once the functionality is in the UI - see https://trello.com/c/PnoMy4LK/73-discard-manual for a ticket about delete_draft_manual.

The Manuals Publisher app uses the govuk_content_production Mongo database in production and govuk_content_development in development. However, there is a manuals_publisher_production Mongo database which is also dumped and restored via the data replication scripts to manuals_publisher_development.

I assume the manuals_publisher_production database is no longer used in which case it would be helpful to remove it; at least from the data replication scripts. It's particularly confusing, because it's easy to assume that the app is using the manuals_publisher_development database locally.

This was originally added as a card in Trello by @floehopper. I've moved it because we've agreed to use GitHub issues instead of Trello.

We should decide whether or not it's worth keeping this script. If we do decide to keep it, we'll need to check it works and ensure that it remains working by adding the appropriate test coverage.

From this comment by @h-lame:

rebuild_major_publication_logs_for_manuals was a "one-off" script in response to a support ticket about the /updates page being wrong for a manual. We were always publishing change notes even for minor updates and so we had to rebuild the PublicationLog entries, that turned out to be quite painful to achieve and this was the result. It has test coverage, and we may need to do it again as it's still unclear if the current PublicationLog entries are correct (certainly at least one ticket has come in to zendesk suggesting they are, but we haven't done anything with them yet)

This has come out of my investigations into #920.

We need to add some kind of validation to check that the user has selected a file.

Steps to replicate

1. Create a manual
2. Add a section
3. Edit the section
4. Click "Add attachment"
5. Click "Save attachment" without adding a file
6. Observe the KeyError: key not found: "file" error message

Using both is confusing and inconsistent. We should investigate whether there's a preferred option across GOV.UK/GDS and make it so.

• https://github.com/alphagov/manuals-publisher/tree/master/spec/features
• https://github.com/alphagov/manuals-publisher/tree/master/features

The app POSTs to /manuals/<guid>/sections/preview to generate a preview for an unsaved section. A GET to the same URL results in a 500 error because I think the app is treating /preview as a Section GUID and therefore tries to render the template for displaying a Section. I think a GET to this URL should probably be a 404.

This exception recently occurred in production and I thought we should investigate whether it's occurring due to a bug in the code.

There's a number of methods defined in features/support/manual_helpers.rb that allow for various things to be created without using the UI:

create_manual_without_ui
create_section_without_ui
edit_manual_without_ui
edit_section_without_ui
publish_manual_without_ui
withdraw_manual_without_ui
create_documents_for_manual_without_ui
republish_manuals_without_ui

I guess this is because creating them via the UI take too much time or the UI doesn't support some of the operations.

One problem with this approach is that these methods tend to reach quite deeply into the rest of the code to work, and when refactoring we have to go and fix these helpers as well as the code we are changing.

It might be possible to remove some of these methods and either make the test suite slower, or find some other way of testing the functionality. This would allow us to rely on these tests as test of behaviour and make refactoring easier.

It's currently impossible to use ActiveRecord::ConnectionAdapters::SchemaStatements methods, e.g. #rename_table, from within a migration, because mongoid_rails_migrations relies on a later version of mongoid. More specifically, Mongoid::Migration#connection relies on Mongoid.default_session rather than Mongoid.database.

Although this change isn't specifically mentioned in the changelog, it looks as if it was made in this commit. Running the following commands suggest that it the first release containing this commit was v3.0.0.

$ git clone [email protected]:mongodb/mongoid.git
$ cd mongoid
$ git tag --contains b5040fc196d0cf4552b4608278a14c72e2f9656f | sort | head -n1
v3.0.0

Note that it may be more appropriate to use a better alternative to the mongoid_rails_migrations gem rather than attempt to fix this.

It looks as though the standard way of defining multiple MongoDB hosts is to use the MONGODB_URI environment variable (see other alphagov apps using this variable). Manuals Publisher uses a custom MONGODB_NODES environment variable instead because Ruby 2.1 couldn't parse multiple hosts from the MONGODB_URI. See the commit note and comments in alphagov/govuk-puppet@0d03bfd for more information.

Manuals Publisher is now using Ruby 2.2 (see a0aa440) so it should hopefully be possible to remove MONGODB_NODES and switch to using MONGODB_URI instead.

PR #822 updated manuals-publisher to use an alphagov fork of mongoid_rails_migrations. The changes in this fork allowed us to fix issue #816 and get the builds passing on Jenkins again.

I'm creating this issue as a reminder that we might want to improve our fix in mongoid_rails_migrations, as per @floehopper's comment in the description of PR #822:

This fix is a bit of a bodge - the proper fix would be along the lines of adacosta/mongoid_rails_migrations#3...

We're seeing these errors on master:

LoadError: cannot load such file -- active_model/translation
/var/lib/jenkins/bundles/manuals-publisher/deployed-to-production/ruby/2.2.0/gems/activemodel-3.2.22.3/lib/active_model/validations.rb:49:in `block in <module:Validations>'
/var/lib/jenkins/bundles/manuals-publisher/deployed-to-production/ruby/2.2.0/gems/activesupport-3.2.22.3/lib/active_support/concern.rb:121:in `class_eval'
/var/lib/jenkins/bundles/manuals-publisher/deployed-to-production/ruby/2.2.0/gems/activesupport-3.2.22.3/lib/active_support/concern.rb:121:in `append_features'

This has also been noted in 0a674b5.

Since alphagov/govuk-puppet@857083c we've been seeing the following warnings when installing a bundle from scratch, e.g. in CI builds like this one.

Unable to use the platform-specific () version of activesupport (3.2.22.3) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of sprockets (2.2.3) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of cucumber (2.2.0) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of railties (3.2.22.3) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of cucumber-rails (1.4.5) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of domain_name (0.5.20161129) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of faraday (0.9.0) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of rest-client (2.0.0) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of oauth2 (1.3.0) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of omniauth (1.4.2) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of rubocop (0.39.0) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of jquery-rails (3.1.4) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of jasmine-rails (0.14.1) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of quiet_assets (1.0.3) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of redis-namespace (1.5.0) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of rspec-expectations (3.2.1) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of rspec-mocks (3.2.1) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of rspec-rails (3.2.3) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.
Unable to use the platform-specific () version of sinatra (1.4.4) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config specific_platform true` and install again.

See discussion in #814.

Arrises from #878

In order to make ManualRelocator easier to use and to remind other developers that the code in the class should not be removed we should create a simple rake task that instantiates and calls the class with the command line arguments.

Some specs are using RSpec spies, but others are using RSpec test doubles. We should chose one approach and use it consistently across the test suite.

PR #945 adds information to the README from the ops manual. It references a script named remove_draft_manual_section that no longer exists in the app. We need to ensure the information in the README matches the code in the app.

• Should we remove the references to the remove_draft_manual_section script from the README? Maybe whatever it was doing can now be done through the UI?
• Should we resurrect the remove_draft_manual_section script?
• Should we replace references to the remove_draft_manual_section script with references to other, more suitable, scripts?

This problem was originally reported in Zendesk ticket 1901209 on 1 Mar 2017.

There are unexpected updates listed for The Highway Code against 1 March 2017.

We expect to see "Annex 5 Penalties" with a single change described as "Updated the penalty table to increase penalty points for using a hand-held mobile phone when driving from 3 to 6.".

We're also seeing:

• Road users requiring extra care (204 to 225)
  • Updated Rule 209 to add an image of the school bus road sign.
• Signals to other road users
  • New section added.
• Signals by authorised persons
  • New section added.
• Road markings
  • New section added.
• Vehicle markings
  • New section added.
• Annex 8. Safety code for new drivers
  • Updated the Pass Plus email address to [email protected]

For all the updates on 1 March 2017 we're seeing two identical change notes, one with a timestamp of 2017-03-01T09:49:18Z and the other with a timestamp of 2017-03-01T11:03:18Z.

This was originally added as a card in Trello by @floehopper. I've moved it because we've agreed to use GitHub issues instead of Trello.

As far as I can see, the CI build creates the database from scratch each time rather than attempting to run the migrations. I'm wondering whether this is sensible and/or standard practice for GOV.UK apps.

At first glance, it doesn't look as if there are any features specific to manuals: https://github.com/alphagov/smokey/tree/master/features.

Todo

• Add manuals-publisher permissions to test signon user
• Add test for manuals-publisher (alphagov/smokey#258)
• Enable notifications for Smokey build failures (alphagov/govuk-puppet#5825)
• Check that we're receiving email notifications for Smokey build failures

Arises from: https://github.com/alphagov/manuals-publisher/pull/900/files/92040afe8491a41d9dab4f32177b2f9c6d074551..b3cae20cb3f9dde4c65901f9f0fba0c1efc4a5b8#diff-de762617bb0811292f68975b2a3674da

It's not clear why this rescue is required and it may hide an spec failure when we're refactoring, so it would be good to remove it.

As per @h-lame's comment on PR #798:

I'd be 100% surprised if we need publishing_api and publishing_api_v2. I suspect we can drop the former, and rename the latter to publishing_api

Update 4 Apr 17

We're no longer using v1 of the Publishing API in manuals-publisher so it can be removed. Once removed we should probably rename Services.publishing_api_v2 to .publishing_api.

Arrises from: #837 (comment)

There are a number of classes with ManualSection in their name, for example ManualSectionIndexableFormatter. As we've settled on Section as the name of the parts that make up a manual, we should rename these classes to be consistent.

Hi!

This is the only application on GOV.UK which is running Ruby 2.1.2. It would be really good to upgrade it so that we can remove that Ruby version.

This section of the Govspeak help partial seems to be specific to the Whitehall app.

Also since this is presumably a copy, is it up-to-date?

We've had two problems caused by missing require statements. One for a builder (01e4ab5) and another for a validator (eea94ce).

I think that everything in app/.rb is auto-required/loaded so it might make sense to extend that to app/**/.rb so that it includes builders and validators too.

The following warning is appearing in the build output:

[DEPRECATED] The 'safe' write concern option has been deprecated in favor of 'w'

It would be good to fix this to improve the signal-to-noise ratio in the test output.

https://github.com/elabs/pundit

https://github.com/alphagov/specialist-publisher/tree/master/app/policies

The idea would be to end up with a single class, Manual which includes Mongoid::Document, etc.

This was originally added as a card in Trello by @floehopper. I've moved it because we've agreed to use GitHub issues instead of Trello.

We feel as though we've seen intermittent failures on CI so I'm opening this issue to allow us to record information about when they happen.

Note that this probably depends on having completed #840.

This was originally added as a card in Trello by @floehopper. I've moved it because we've agreed to use GitHub issues instead of Trello.

Multiple instances of the following deprecation warning are appearing in the build output, e.g. in this build:

[DEPRECATION] `last_comment` is deprecated.  Please use `last_description` instead.

It would be good to fix this to improve the signal-to-noise ration in the build output.

(This is mostly a brain dump following some investigation on 2nd line.)

Following the Zendesk ticket https://govuk.zendesk.com/agent/tickets/1210622 it was found that the following govspeak is considered invalid by Specialist Publisher:

* [InlineAttachment:DHPC_Drug_shortage_InductOs_GB-NIR_CLEAN_FINAL.pdf]: Some text

After some investigation the error seems to happen only when all of the following are true:

• There is a list item that begins with a square bracket * [
• There is a closing square bracket immediately followed by a colon and a space: ]:

Putting a character between the * and the [ works, eg * .[. Removing the colon works. Using other characters that might be considered special, such as $ or . works fine. It was also noted that \: seems to escape the colon.

The shortest syntax that produces this bug is:
* [a]: (note the space)

As per this comment.

We should decide whether or not it's worth keeping this script. If we do decide to keep it, we'll need to fix it and ensure that it remains fixed by adding the appropriate test coverage.

From this comment by @floehopper:

The two places which are using v1 of the Publishing API have been broken since this commit, because they rely on methods which were removed from gds-api-adapters in v38.0.0; the application is currently using v39.2.0. This PR removes a bunch of code around the invocations of these non-existent methods on the basis that the code is currently useless.

[...]

I'm slightly less confident that the bin/reslug_manual_section script and its associated code can be removed, but given that it is currently broken, if someone wants to keep it, it will need to be fixed. I would also argue that it should only be kept if we write a test for it to ensure that it stays in a working state.

From this comment by @h-lame:

I think you could replace the put_content_item call in PublishingAPIRedirecter with a call to put_content on a publishing_api_v2 adapter. I'm fairly sure these are equivalent.

The bin/reslug_manual_section script is something we wrote last year in response to a support ticket, and I think we've had to run it more than once (but obvs not since I broke it with a gem bump). We get a lot of requests to do this kind of thing to HMRC manuals (which live in a different app), and it's conceivable we'd get other requests to do it on these, as you can't change the slug once it's published (I think).

I think it's probably 50:50 on whether we drop that script as you've done already, or fix the underlying thing and add tests. If we chose to fix it I suspect it would be worth looking at the ManualRelocator in lib. This object works across an entire manual rather than a single section, but it's more recent, and has some tests. Maybe a good approach would be to remove the PublishingAPIRedirecter, and replace it with some functionality extracted from ManualRelocator? I reckon it's up to you as current custodians though, as that sounds like a lot of work.

PR #837 renames SpecialistDocumentEdition to SectionEdition but the data is still stored in the specialist_document_editions Mongo collection.

We should rename the collection to section_editions once PR #837 has been merged.

As suggested in #849, once that PR is merged, it should be safe to unset the field for all existing persisted instances.

We should decide whether or not it's worth keeping this script. If we do decide to keep it, we'll need to check that it works and ensure that it remains working by adding the appropriate test coverage.

From this comment by @h-lame:

republish_manuals is useful for when all the content needs to be resent to the publishing-api when we change the schema or so-on (for example we might need to use this if we implement https://trello.com/c/Q01pcyd3/103-send-more-details-to-rummager-from-manuals-publisher)

Hey,

just putting a note in for this.

there seems to be some flaky tests in the manuals publisher that get triggered when you update content schema, looking through the builds it seems to be a common flaky test

As per @h-lame's comment on PR #798.

It feels like it might be a bug that this and the ManualDocumentRenderer you deal with in the next few commits are subtly different; this one doesn't do FootnoteSomethingSomething in its pipeline, but the ManualDocumentRenderer does. We should check obviously, but I think we can get rid of this one and replace it's single use with an instance of the ManualDocumentRenderer instead.