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 for upgrade with missing journal #796

Merged
merged 4 commits into from Mar 21, 2020
Merged

Fix for upgrade with missing journal #796

merged 4 commits into from Mar 21, 2020

Conversation

dbxnr
Copy link
Contributor

@dbxnr dbxnr commented Jan 10, 2020

Fix for #690 - if a journal is found to be missing during upgrade the user will be prompted to create it or cancel the upgrade.

Tests depend on #795

Checklist

  • The code change is tested and works locally.
  • Tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?

Copy link
Member

@wren wren left a comment

Choose a reason for hiding this comment

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

This is looking good. Just a few comments inline.

jrnl/upgrade.py Show resolved Hide resolved
jrnl/upgrade.py Show resolved Hide resolved
features/upgrade.feature Outdated Show resolved Hide resolved
"default_hour": 9,
"timeformat": "%Y-%m-%d %H:%M",
"linewrap": 80,
"encrypt": false,
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 add the same tests but for an encrypted journal? Just be sure it works.

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've added a test for this which is failing, although the actual code works. I'm not certain, but it could be that the output it being piped to stderr which doesn't have an equivalent "the output should contain". Can you take a look? I'll add a step in core.py if so.

features/upgrade.feature Outdated Show resolved Hide resolved
@wren wren changed the base branch from develop to temp-missing-journal March 21, 2020 21:58
@wren wren merged commit 52dc3ac into jrnl-org:temp-missing-journal Mar 21, 2020
wren added a commit to wren/jrnl that referenced this pull request Mar 28, 2020
* Fix for upgrade with missing journal
* add test, refactor solution
* add missing test config

Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>
wren added a commit that referenced this pull request Mar 28, 2020
* Fix for upgrade with missing journal
* add test, refactor solution
* add missing test config

Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>
@wren wren added the bug Something isn't working label Mar 28, 2020
wren added a commit that referenced this pull request Apr 18, 2020
* Fix for upgrade with missing journal
* add test, refactor solution
* add missing test config

Co-authored-by: Jonathan Wren <jonathan@nowandwren.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants