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

Fix the issue of download plans (only coverpage can be downloaded) #155

Merged
merged 6 commits into from
Mar 4, 2022

Conversation

pengyin-shan
Copy link

Fixes part of #153 .

This PR does not include JSON issue (unique on production and staging) and functional tests. JSON issue might be a configuration issue, will do further navigation after this emergency release. The functional test will come soon.

@pengyin-shan pengyin-shan requested a review from lagoan March 4, 2022 20:28
CHANGELOG.md Outdated
@@ -1,5 +1,8 @@
# Changelog

## [3.0.4+portage-3.0.5]
- Fixed pdf download issue
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to add this on the CHANGELOG under a new version.
Also, we need to be under a subheading for FIXED.
Now that we are keeping better track of issues we should start adding links to the issue being fixed #153

You can see jupiter's CHANGELOG as an example

Copy link
Author

Choose a reason for hiding this comment

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

Working on it : ) should I put today as the release date?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please

@@ -111,7 +111,7 @@
options_for_select(Settings::Template::VALID_MARGIN_RANGE.to_a,
@export_settings.formatting[:margin][:right]),
class: 'form-control',
"data-default": @plan.template.settings(:export).formatting[:margin][:rigth] %>
"data-default": @plan.template.settings(:export).formatting[:margin][:right] %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

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

Thing are looking good. Just some minor comments made there about the change log.

We should also add a skipped stub for testing so we have a reference of where we need to work on this next.

@@ -71,4 +71,4 @@
end

# Used by Rails' routes url_helpers (typically when including a link in an email)
Rails.application.routes.default_url_options[:host] = "localhost:3000"
# Rails.application.routes.default_url_options[:host] = "localhost:3000"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line part of the problem for this issue?

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

1 Warning
⚠️ There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior.

Generated by 🚫 Danger

Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

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

LGTM!

@pengyin-shan pengyin-shan merged commit 74239cb into integration Mar 4, 2022
@lagoan lagoan deleted the issue153 branch March 4, 2022 22:17
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.

2 participants