{"push_id":1422565761,"size":5,"distinct_size":1,"ref":"refs/heads/remove-extra-params-in-urls","head":"3f018513b47200cf746d623b4a706f9d89c083d4","before":"18c9880567d41a244ce12052605e6afa11015211","commits":[{"sha":"10d2a1784727e6b7197d0680a26e2e0d8838d0e6","author":{"name":"Murray Steele","email":"7fd5846ea6ef627614ea669595ce6a868b4c0162@h-lame.com"},"message":"Tighten up PublicationLog#change_notes_for\n\nThis method would return all publication logs that had the supplied slug\nas a prefix to allow us to get the publication logs for a manual section\nat the same time as the publication logs for the manual.  However this\ndidn't account for manuals whose slugs might be a prefix of another\nmanual.  For example the manual slug 'guidance/test' is a prefix of the\nmanual slug 'guidance/test-manual'.  This means when asking for the logs\nfor 'guidance/test' you would get the logs for both manuals (and all their\nsections) which is almost certainly not what you wanted.\n\nWe tighten up the regexp used to extract the logs so that the slug is\nfollowed by a / (so we get logs for child sections) or the end of the\nstring (so we get logs for the manual).\n\nThis has happened as there are a handful of manuals whose slugs are a\nprefix of another manual, but in practice they're all either withdrawn\nor unpublished, so there are no logs.  But it's likely to become a bigger\nproblem if we don't fix it.","distinct":false,"url":"https://api.github.com/repos/alphagov/manuals-publisher/commits/10d2a1784727e6b7197d0680a26e2e0d8838d0e6"},{"sha":"7a3a95231ffc23e1c2c82192addbd64d2fa30e88","author":{"name":"Murray Steele","email":"7fd5846ea6ef627614ea669595ce6a868b4c0162@h-lame.com"},"message":"Merge pull request #773 from alphagov/fix-publication-log-change-notes-for\n\nTighten up PublicationLog#change_notes_for","distinct":false,"url":"https://api.github.com/repos/alphagov/manuals-publisher/commits/7a3a95231ffc23e1c2c82192addbd64d2fa30e88"},{"sha":"69c7d0947dfa450a1fb0dd6d60e34ac81e9d84d6","author":{"name":"Neil van Beinum","email":"3caa7df0d267bb3419d49e464d9bc1484cb9c247@unboxedconsulting.com"},"message":"Fix a spec which did not call intended method","distinct":false,"url":"https://api.github.com/repos/alphagov/manuals-publisher/commits/69c7d0947dfa450a1fb0dd6d60e34ac81e9d84d6"},{"sha":"3470cef66bc487c7983be38370b3b492323ce962","author":{"name":"Murray Steele","email":"7fd5846ea6ef627614ea669595ce6a868b4c0162@h-lame.com"},"message":"Merge pull request #775 from alphagov/publish_spec_fix\n\nFix a spec which did not call intended method","distinct":false,"url":"https://api.github.com/repos/alphagov/manuals-publisher/commits/3470cef66bc487c7983be38370b3b492323ce962"},{"sha":"3f018513b47200cf746d623b4a706f9d89c083d4","author":{"name":"Murray Steele","email":"7fd5846ea6ef627614ea669595ce6a868b4c0162@h-lame.com"},"message":"Fix url generation to remove extra query string params\n\nUrls in the system were being generated with path params duplicated\nas query params as follows:\n\n    /manuals/<manual_id>?id=<manual_id>\n    /manuals/<manual_id>/sections/<section_id>?id=<section_id>&manual_id=<manual_id>\n\nAfter some digging we discovered that this was because the objects we were\nusing for url generation were not the ManualRecord or\nSpecialistDocumentEdition mongoid documents, or even the Manual and\nSpecialistDocument model objects.  Instead they were deeply wrapped up in\nSimpleDelegator classes like NullValidator or ManualWithDocuments.\n\nThe rails routing algorithm uses the provided arguments to fill in the\npath segments in the url, but does equality to decide if the argument\nhas been consumed and should be removed for consideration later when\nbuilding query string params.  The SimpleDelegator objects mean that\nthe manual and section instances were not considered equal and so were\nnot removed from the list of arguments used already in building the\nurl and so ended up in the query string as well as the path.  We can\nsee the SimpleDelegator and eql? behaviour below:\n\n    class A; end\n    a1 = A.new\n    a2 = A.new\n    a1.eql? a2 #=> false\n    a1.eql? a1 #=> true\n\n    class B < SimpleDelegator; end\n    ba1 = B.new(a1)\n    ba2 = B.new(a2)\n    ba1.eql? ba2 #=> false\n    ba1.eql? ba1 #=> false\n    ba1.eql? a1 #=> true\n\nThe delegate thinks it is equal to the delegated object, but does not\nthink it is equal to another delegate - even when that other delegate is\nitself.  This is because the delegate delegates eql? to the wrapped object\nand this does object equality, so really `ba1.eql? ba1` is doing\n`a1.eql? ba1`.  If instead we define our own eql? method on the wrapped\nobject we get the following behaviour:\n\n   class C\n     attr_reader :id\n     def initialize(id)\n       @id = id\n     end\n     def eql?(other)\n       @id.eql? other.id\n     end\n   end\n\n   c1 = C.new(1)\n   c1_2 = C.new(1)\n   c2 = C.new(2)\n   c1.eql? c1 #=> true\n   c1.eql? c1_2 #=> true\n   c2.eql? c1 #=> false\n\n   bc1 = B.new(c1)\n   bc1_2 = B.new(c1_2)\n\n   bc1.eql? bc1 #=> true\n   bc1.eql? c1 #=> true\n   bc1_2.eql? c1 #=> true\n   bc1.eql? c1_2 #=> true\n   bc1.eql? bc1_2 #=> true\n\nSo this is what we do, we add eql? implementations to Manual and\nSpecialistDocumentEdition to make sure they are considered equal,\nnot matter how deeply wrapepd in simple delegators they are.\n\nThere's a closed (but not fixed) issue report on journey (the rails\nrouting algorithms) that explains this situation here:\n\nhttps://github.com/rails/journey/issues/29","distinct":true,"url":"https://api.github.com/repos/alphagov/manuals-publisher/commits/3f018513b47200cf746d623b4a706f9d89c083d4"}]}
{"actor":{"display_login":"h-lame"}}