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

Add resource thumbnails on generation page #642

Merged
merged 49 commits into from Oct 27, 2017

Conversation

2 participants
@JackMorganNZ
Member

JackMorganNZ commented Oct 25, 2017

This pull request adds thumbnail images to the resource generation page. When a user changes the requested resource combination on the generation form, the preview image updates. Adds management commands to the pipeline as well. Updates all resources for this change.

Also improves CI testing by using smarter conditionals to only test required jobs (rather than creating job, spooling up environment, then detecting to skip job).

Fixes #632
Fixes #640

JackMorganNZ added some commits Oct 24, 2017

Add management command makeresourcethumbnails
This command creates thumbnail images of PDF resources
and stores them in the 'build' folder for 'collect_static'
to find.

Next steps:
- Only display one page per resource.
- Allow resource to specify which page to display as thumbnail.
- Add in frontend connections to display thumbnails.
Require resources of more than one page to designate thumbnail page
If no page is specified, a custom exception is thrown.

JackMorganNZ added some commits Oct 25, 2017

@codecov

This comment has been minimized.

codecov bot commented Oct 25, 2017

Codecov Report

Merging #642 into develop will increase coverage by 1.73%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #642      +/-   ##
===========================================
+ Coverage     85.8%   87.53%   +1.73%     
===========================================
  Files           84       88       +4     
  Lines         2219     2303      +84     
  Branches       271      283      +12     
===========================================
+ Hits          1904     2016     +112     
+ Misses         253      231      -22     
+ Partials        62       56       -6
Impacted Files Coverage Δ
...ed/resources/views/ParityCardsResourceGenerator.py 100% <ø> (ø) ⬆️
...sources/views/BinaryCardsSmallResourceGenerator.py 100% <100%> (ø) ⬆️
...gged/utils/errors/MoreThanOneThumbnailPageFound.py 100% <100%> (ø)
...rces/views/SortingNetworkCardsResourceGenerator.py 98.93% <100%> (ø) ⬆️
.../resources/management/commands/_ResourcesLoader.py 100% <100%> (+14.28%) ⬆️
...resources/views/SearchingCardsResourceGenerator.py 100% <100%> (ø) ⬆️
...d/resources/views/TreasureHuntResourceGenerator.py 100% <100%> (ø) ⬆️
csunplugged/config/settings/base.py 100% <100%> (ø) ⬆️
csunplugged/utils/errors/ThumbnailPageNotFound.py 100% <100%> (ø)
...ged/resources/management/commands/loadresources.py 100% <100%> (ø) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e630b1e...39d5c18. Read the comment docs.

JackMorganNZ added some commits Oct 26, 2017

@@ -158,15 +158,16 @@
# STATIC FILE CONFIGURATION
# ------------------------------------------------------------------------------
# See: https://docs.djangoproject.com/en/dev/ref/settings/#static-root
STATIC_ROOT = str(ROOT_DIR.path("staticfiles"))
STATIC_ROOT = str(ROOT_DIR.path("staticfiles")) + "/"

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 27, 2017

Contributor

Also, probably better to do os.path.join(ROOT_DIR.path("staticfiles"), "") - that will add slash only if it is missing.

]
STATIC_ROOT = str(ROOT_DIR.path("staticfiles")) + "/"

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 27, 2017

Contributor

Remove duplicate

@@ -45,6 +46,9 @@ def load(self):
except KeyError:
raise MissingRequiredFieldError()
# Check resource template file exists
open(os.path.join("templates/", resource_template), encoding="UTF-8")

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 27, 2017

Contributor

os.path.join("templates", resource_template) - note removed slash

# TODO: Import repeated in next for loop, check alternatives
empty_generator = get_resource_generator(resource.generator_module)
combinations = resource_valid_test_configurations(
empty_generator.valid_options,
header_text=False
)
progress_bar = tqdm(combinations, ascii=True)

This comment has been minimized.

@jordangriffiths01
css_file = finders.find("css/print-resource-pdf.css")
css_string = open(css_file, encoding="UTF-8").read()
base_css = CSS(string=css_string)
return (html.write_pdf(stylesheets=[base_css]), filename)
def generate_resource_copy(generator):
def generate_resource_copy(generator, thumbnail=False):
"""Retrieve data for one copy of resource from resource generator.

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 27, 2017

Contributor

Raises ThumbnailPageNotFound

@@ -150,6 +153,12 @@ def generate_resource_copy(generator):
elif paper_size == "letter":
max_pixel_height = 249 * MM_TO_PIXEL_RATIO
if thumbnail and len(data) > 1:
try:
data = [next(page for page in data if page.get("thumbnail", False))]

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 27, 2017

Contributor

This is pretty hard to parse. Perhaps consider

data = list(filter(lambda data: data.get("thumbnail"), data))

This will filter the list to only include pages where thumbnail is True. You can then do a length check, and raise an error if empty (no thumbnail listed) or more than one page was marked.

box-sizing: inherit;
}
// html {

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 27, 2017

Contributor

Are these commented out intentionally?

}
url = reverse("resources:resource", kwargs=kwargs)
response = self.client.get(url)
self.assertEqual(200, response.status_code)

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 27, 2017

Contributor

Looks like a few that got missed for http.HTTPStatus.OK etc.

JackMorganNZ added some commits Oct 27, 2017

@JackMorganNZ JackMorganNZ merged commit 6c354e1 into develop Oct 27, 2017

4 checks passed

codecov/patch 100% of diff hit (target 85.8%)
Details
codecov/project 87.53% (+1.73%) compared to e630b1e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@JackMorganNZ JackMorganNZ deleted the issue/632 branch Oct 27, 2017

@JackMorganNZ JackMorganNZ referenced this pull request Oct 30, 2017

Merged

Release 2.0.0 alpha.5 #657

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment