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 save templated journal entries if the received raw text is the same as the template itself #1653

Merged
merged 8 commits into from
Feb 11, 2023

Conversation

Briscoooe
Copy link
Contributor

@Briscoooe Briscoooe commented Dec 21, 2022

…same as the template itself

Checklist

  • I have read the contributing doc.
  • I have included a link to the relevant issue number.
  • I have checked to ensure there aren't other open pull requests
    for the same issue.
  • I have written new tests for these changes, as needed.

Issue number #1652

I did not add tests for this as I wasn't sure if adding a blocking processes for testing the external editor was the correct thing to do. I also noticed there are currently no tests that test templates. Let me know if this is correct and I can add.

One thing that's not optimised about the PR is that _get_editor_template function, which performs file I/O, is called twice:

  • Once to get the template text (original code)
  • Once again to get the template text and check if its the same as the received raw text (my added code)

The alternative to calling it twice is changing the _write_in_editor and/or write_mode function(s) to read the template in once at the start and pass it as a variable. I can make this change if necessary but thought it was safer to just check again and allow the duplicate I/O operation.

@micahellison micahellison mentioned this pull request Jan 7, 2023
4 tasks
@wren
Copy link
Member

wren commented Jan 7, 2023

@Briscoooe I just wanted to say that, even though this is a small PR, we really appreciate the thoughtfulness that's clearly apparent in your contribution. Your filed issue was clear and concise, your considerations in this PR are right on the mark, and you match the style of the codebase perfectly. Thank you, and I hope you'll consider continuing to contribute to jrnl!

Copy link
Member

@micahellison micahellison left a comment

Choose a reason for hiding this comment

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

Thanks for this. Looks great to me so far, and I appreciate your thoughtful comments about this issue.

I think the performance issue you mentioned is acceptable, since jrnl already has much larger I/O performance issues (like loading an entire journal even when only a subset of the data needs to be retrieved) that I don't think this would ever be noticeable to a user. However, if you'd like to tackle it, you're more than welcome to do so.

I do think a test is necessary for this PR to be complete, however. I've just added a template test feature file and it's been merged into develop. Feel free to add on a test to that file for this, and let us know if you'd like any help with it.

@Briscoooe
Copy link
Contributor Author

First, thanks @wren and @micahellison! Huge fan of the project and happy to continue contributing.

And @micahellison I see the test feature you've added but my changes cause all of them to fail, which is actually the intended behaviour on my end. The expected output of the test is simply the contents of the template, unchanged.

Is there any way to make a templated entry, with additions, from the command line without requiring an external editor? i.e. something like:

$ jrnl --config-override template features/templates/basic.template "hello world"

Which would make the entry:

This text is in the basic template
hello world

Without this I'm not sure how my PR could pass this test fixture

@micahellison
Copy link
Member

I see the test feature you've added but my changes cause all of them to fail, which is actually the intended behaviour on my end. The expected output of the test is simply the contents of the template, unchanged.

I see what you mean. Do feel free to modify the test as necessary, since this PR is very much intended to change that behavior. I just wanted to get some scaffolding in there for further template testing.

Is there any way to make a templated entry, with additions, from the command line without requiring an external editor?

No, there isn't. Maybe there should be, but that would be a separate enhancement.

I think to get this PR covered by tests, there needs to be a version of the current template test that ensures no entries are modified, plus another similar test that confirms entries are modified when using this step:

        And we append to the editor if opened
            text to append to the editor            

There's a good example of this in write.feature. Once that's applied to this test, it should be passing with the new functionality you've added.

@Briscoooe Briscoooe reopened this Jan 16, 2023
@Briscoooe
Copy link
Contributor Author

Briscoooe commented Jan 16, 2023

My bad I temporarily discarded my changes during merge, accidentally closing the PR. Reopening.

Can you expand on what you mean by there needs to be a version of the current template test that ensures no entries are modified? The current tests check that entries are created. Are you suggesting modifying existing entries while overriding the template config? i.e. something like

jrnl --config-override features/templates/basic.template -edit -1

@micahellison
Copy link
Member

Can you expand on what you mean by there needs to be a version of the current template test that ensures no entries are modified?

Sorry, I should've said "no entry" rather than "no entries." Basically, it should test the "happy path" of this feature, that if the entry is the same as the template, the entry isn't saved to the journal.

Then there should be an opposite test that confirms that when the entry differs from the template, the entry is saved to the journal.

@Briscoooe
Copy link
Contributor Author

Briscoooe commented Feb 1, 2023

Edit

In pursuit of understanding how the write to the editor if opened step works I stumbled across the mock_editor and noticed that it has an "append" mode. Problem solved! Leaving the original comment underneath for visibility.


Then there should be an opposite test that confirms that when the entry differs from the template, the entry is saved to journal.

This is the test I can't produce. The changes in my PR cause the current version of this test (Template contents should be used in new entry) to fail as. Unless I'm missing something, there's no way to add additional text to a template during a test due to the lack of an external editor for input

Here is the current test:

    Scenario Outline: Template contents should be used in new entry
        Given we use the config "<config_file>"
        And we use the password "test" if prompted
        When we run "jrnl --config-override template features/templates/basic.template"
        And we run "jrnl -1"
        Then the output should contain "This text is in the basic template"

        Examples: configs
        | config_file          |
        | basic_onefile.yaml   |
        | basic_encrypted.yaml |
        | basic_folder.yaml    |
        | basic_dayone.yaml    |

This will always fail for my feature as there are no changes made to the template. The test attempts to simply save the unmodified template as an entry. To try and change the templated entry contents during the test I tried some of the write to the editor if open stuff from the write.feature file but it appears to do a full replace, not an append.

Here is what I mean.

        .....
        And we write to the editor if opened
            This is an addition to a templated entry
        When we run "jrnl --config-override template features/templates/basic.template"
        And we run "jrnl -1"
        Then the output should contain "This text is in the basic template"
        Then the output should contain "This is an addition to a templated entry"
        .....

And the output:

AssertionError: 
  EXPECTED:
  This text is in the basic template
  
  ACTUAL STDOUT:
  2023-02-01 21:50 This is an addition to a templated entry
  
  
  
  ACTUAL STDERR:
  
assert (('This text is in the basic template' in '2023-02-01 21:50 This is an addition to a templated entry\n\n') == True or ('This text is in the basic template' in '') == True)

You can see that if you try and write to the editor if opened and also use a template, it completely overrides the template.

I can easily write a happy path test for this feature as the current template test already satisfies it. The problem is now successfully saving a templated entry, with changes from the original template, during a test.

Copy link
Member

@micahellison micahellison left a comment

Choose a reason for hiding this comment

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

The tests look great! Thanks for your work on this.

@micahellison micahellison changed the title Don't save templated journal entries if the received raw text is the … Don't save templated journal entries if the received raw text is the same as the template itself Feb 11, 2023
@micahellison micahellison added the bug Something isn't working label Feb 11, 2023
@micahellison micahellison merged commit b41a988 into jrnl-org:develop Feb 11, 2023
wren pushed a commit to wren/jrnl that referenced this pull request Mar 4, 2023
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.

Templated entries should not be saved if the raw text is identical to the original template
3 participants