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

Lookup by base paths v2 #822

Closed
wants to merge 2 commits into from
Closed

Lookup by base paths v2 #822

wants to merge 2 commits into from

Conversation

MatMoore
Copy link
Contributor

@MatMoore MatMoore commented Mar 6, 2017

This request replaces /lookup-by-base-path. It allows draft and unpublished documents to be looked up in addition to live content.

More background in #812

Trello: https://trello.com/c/6VPQcc7C

@MatMoore MatMoore changed the title [WIP] Lookup by base paths v2 Lookup by base paths v2 Mar 8, 2017
@MatMoore
Copy link
Contributor Author

MatMoore commented Mar 8, 2017

I've had to park this ticket for a while but the publishing api part is ready for review.

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks good @MatMoore thanks for putting it together, here's a few suggestions.

@@ -126,7 +126,7 @@ You can run the pact verification tests on their own using:
$ bundle exec rake pact:verify
```

See [doc/pacts.md](doc/pacts.md) for more details about the pacts and the pact
See [doc/pact_testing.md](doc/pact_testing.md) for more details about the pacts and the pact
Copy link
Member

Choose a reason for hiding this comment

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

👍

private

def base_paths
params.fetch(:base_paths)
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could use Strong Parameters here (disclaimer: it's a been a while since I used them so this could be a duff suggestion) - perhaps they'd allow us to require an array of base_paths which would save us worrying about a couple of edge cases of bad usage?

documents.content_id,
documents.locale,
editions.document_type,
CASE editions.state WHEN 'draft' THEN 'draft' ELSE 'live' END,
Copy link
Member

Choose a reason for hiding this comment

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

you probably want to use editions.content_store here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ that's a lot simpler 😄

module LookupByBasePaths
def self.call(base_paths)
base_paths_and_content_ids = Edition.with_document
.left_outer_joins(:unpublishing)
Copy link
Member

Choose a reason for hiding this comment

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

we have a with_unpublishing scope here, whether that makes it feel more readable or not though ¯\_(ツ)_/¯

.left_outer_joins(:unpublishing)
.left_outer_joins(:access_limit)
.where(base_path: base_paths)
.where("access_limits.edition_id IS NULL")
Copy link
Member

Choose a reason for hiding this comment

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

I think we can group these together like where(base_path: base_paths, "access_limits.edition_id": nil)

.left_outer_joins(:access_limit)
.where(base_path: base_paths)
.where("access_limits.edition_id IS NULL")
.where("state != 'superseded'")
Copy link
Member

Choose a reason for hiding this comment

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

touch more Railsy to be .where.not(state: "superseded") you might want that to actually be .where.not(content_store: nil) though

.where("state != 'superseded'")
.order(:base_path)
.order(:state)
.order(user_facing_version: :desc)
Copy link
Member

Choose a reason for hiding this comment

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

can we group these?

editions.document_type,
CASE editions.state WHEN 'draft' THEN 'draft' ELSE 'live' END,
unpublishings.type,
unpublishings.alternative_path
Copy link
Member

Choose a reason for hiding this comment

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

If you're looking for where the redirect unpublishings have moved to this value is not the one to use anymore (unfortunately as it was more simple) we now have a hash of redirects which has each type of redirect route in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok. I can rework this to return the hash instead.

@@ -8,24 +8,44 @@

abort "Refusing to run outside of development" unless Rails.env.development?

benchmarks = Location.order("RANDOM()").limit(10).pluck(:base_path)
benchmarks = Edition.order("RANDOM()").limit(10).pluck(:base_path)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks 👍


lookups = base_paths_and_content_ids.group_by(&:first)

lookups.each_with_object({}) do |group, result|
Copy link
Member

Choose a reason for hiding this comment

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

maybe here we would do better looping through the base_paths and adding results, which would allow us to return nil for base_paths that aren't in the Publishing API rather than the more opaque omission?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just went with omission here to match the semantics of v1. Do you think its useful to see the nils?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, since you're providing it with a list of things to look up it feels more semantically helpful to return results that correspond to the list you provided

@MatMoore
Copy link
Contributor Author

MatMoore commented Mar 9, 2017

@kevindew thanks for reviewing. I might be a while fixing the suggestions as I've had to pick up another ticket, but will make the changes once I have a free moment.

This allows draft and unpublished documents to be looked up in addition
to live content.

See #812

Locale and content id are returned for each successful lookup, as well as
unpublishing details if applicable.

I've also included document type, because there are some document types that
the application may not want. For example it makes no sense to tag something to
something with document type "redirect".
@MatMoore
Copy link
Contributor Author

@kevindew I've made the suggested changes.

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

looks good 👍

Various improvements to the V2 lookup-by-base-path endpoint:

- Return nil for missing base paths
- Use content_store instead of state to distinguish draft/live
- Use redirects instead of alternative_path
- Ensure base_paths param is an array
- Group together activerecord method calls
- Use named scopes in query
@MatMoore
Copy link
Contributor Author

FYI I've just pushed an update to the doc/api.md documentation, since I decided not to add a pact test for this endpoint.

I'll wait for alphagov/gds-api-adapters#678 to be reviewed before merging.

@tijmenb
Copy link
Contributor

tijmenb commented Mar 22, 2017

We're picking up https://trello.com/c/6VPQcc7C next week, closing in the meantime.

@tijmenb tijmenb closed this Mar 22, 2017
tomsabin pushed a commit that referenced this pull request Dec 12, 2017
Earlier in the year, work was paused on the /v2/lookup-by-base-path
request [1]. This sceanario has been added to the lookups spec and we
record what the response is with and without the recently added
`with_drafts` paramater.

This parameter is optional, and with its default value, it does not
change the response from the original. When the request is made with the
`with_drafts` parameter as true, then draft documents take precedence
over published.

[1] #822
tomsabin pushed a commit that referenced this pull request Dec 12, 2017
Earlier in the year, work was paused on the /v2/lookup-by-base-path
request [1]. This sceanario has been added to the lookups spec and we
record what the response is with and without the recently added
`with_drafts` paramater.

This parameter is optional, and with its default value, it does not
change the response from the original. When the request is made with the
`with_drafts` parameter as true, then draft documents take precedence
over published.

[1] #822
tomsabin pushed a commit that referenced this pull request Dec 12, 2017
Earlier in the year, work was paused on the /v2/lookup-by-base-path
request [1]. This sceanario has been added to the lookups spec and we
record what the response is with and without the recently added
`with_drafts` paramater.

This parameter is optional, and with its default value, it does not
change the response from the original. When the request is made with the
`with_drafts` parameter as true, then draft documents take precedence
over published.

[1] #822
@barrucadu barrucadu deleted the lookups_v2 branch April 27, 2018 12:55
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.

3 participants