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

Don't display Document Collection withdrawn documents #205

Merged
merged 1 commit into from Dec 14, 2016

Conversation

tvararu
Copy link
Contributor

@tvararu tvararu commented Dec 13, 2016

This brings government-frontend in line with Whitehall, which doesn't show withdrawn documents.

Trello:
https://trello.com/c/IFfWVlb6/533-filter-out-withdrawn-documents-in-document-collections-medium

Follow-up to:

Depends on:

Before

Below screenshots use the fictional /government/collections/with-withdrawn-links-documents example I created that has Financial sanctions, Somalia marked as withdrawn.

screen shot 2016-12-13 at 16 00 32

After

screen shot 2016-12-13 at 16 00 38

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-205 December 13, 2016 16:02 Inactive

groups_without_withdrawn_documents = presenter.groups.first["documents"]

number_of_documents_that_have_withdrawn_false = 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not happy with this but because both group_documents and documents_hash are private I couldn't figure out a neater way to check that the withdrawn ones have been excluded. The only downside with doing this is if somebody goes and modifies the example by say, adding another document, 99% chance is they will break these tests. However, that happens rare enough that this should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

And then expected_number_of_presented_documents

Copy link
Contributor

@gpeng gpeng 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 apart from the naming which, as we all know, is hard 😄

"document_collection_with_withdrawn_links_documents"
)

groups_without_withdrawn_documents = presenter.groups.first["documents"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this name correctly describes what is returned here. This returns (or should return for the test to pass) a collection of non-withdrawn documents. Maybe non_withdrawn_documents might be a better name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or possibly just presented_documents? The withdrawn bit is redundant as that is in the name of the test.


groups_without_withdrawn_documents = presenter.groups.first["documents"]

number_of_documents_that_have_withdrawn_false = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

And then expected_number_of_presented_documents

@tvararu tvararu force-pushed the filter-out-withdrawn-document-collection-links branch from 69cba15 to d7c74df Compare December 13, 2016 16:49
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-205 December 13, 2016 16:49 Inactive
@tvararu tvararu force-pushed the filter-out-withdrawn-document-collection-links branch from d7c74df to 01bd7f4 Compare December 14, 2016 10:15
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-205 December 14, 2016 10:15 Inactive
@gpeng gpeng merged commit 33642a4 into master Dec 14, 2016
@gpeng gpeng deleted the filter-out-withdrawn-document-collection-links branch December 14, 2016 15:43
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.

None yet

3 participants