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

Bugfix: Fix Simplenote importer not titling multi-line documents #2798

Merged
merged 2 commits into from Jan 27, 2024

Conversation

mmorella-dev
Copy link
Contributor

@mmorella-dev mmorella-dev commented Jan 24, 2024

SimplenoteConverter has a bug which causes it to only parse a title in notes which have exactly two lines. This is one of the issues addressed by jamesgecko/simple-to-standard.

Example data:

{
  "activeNotes": [
    {
      "id": "bb33231fc77e49e9b2f66b5d07b1159d",
      "content": "Title One\r\nThis note has a title and one line of body!",
      "..."
    },
    {
      "id": "53c79c16-896f-43e5-80e5-a0f92c85330d",
      "content": "Title Two\r\nThis note has a title and...\r\nmultiple lines...\r\nof body!",
     "..."
    },
    {
      "id": "1b105f4d24534d329ca8d99439219685",
      "content": "\r\nThis note has only a body!",
      "..."
    },
  ],
  "..."
}

Current behavior: the second note is given a title.
Screenshot of Standard Notes running locally. The second note imported with the date as its title.

This PR fixes that issue by replacing an instance of String.prototype.split with a new helper function, splitOnce, which splits a string only on the first occurrence of the delimiter.

After fix: multi-line notes are titled correctly.
Screenshot of Standard Notes running locally. The second note imported with the correct title.

Copy link
Member

@amanharwara amanharwara left a comment

Choose a reason for hiding this comment

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

Also, please add a test case to SimplenoteConverter.spec.ts. Hard to know the change of behavior without that.

mmorella-dev added a commit to mmorella-dev/standardnotes-app that referenced this pull request Jan 26, 2024
From standardnotes#2798 (comment)

Co-authored-by: Aman Harwara <amanharwara@protonmail.com>
@mmorella-dev mmorella-dev force-pushed the fix-simplenote-titles branch 2 times, most recently from 58215ee to ba5e69d Compare January 26, 2024 19:11
@mmorella-dev mmorella-dev force-pushed the fix-simplenote-titles branch 2 times, most recently from ec7ec1e to a7f090f Compare January 26, 2024 19:19
@mmorella-dev
Copy link
Contributor Author

mmorella-dev commented Jan 26, 2024

@amanharwara Thanks for the suggestions! I've made some stylistic changes in-line with your suggestions, and modified the test data to include a note with more than 2 lines. In my testing, this case fails on the unpatched version (79951c3) and passes after my changes (27e7815).

(Sorry for the noise with all the force-pushes. I can get a bit carried away making history "perfect".)

SimplenoteConverter will now parse a title for any note with multiple
lines, not just those with exactly two.

Adds a string helper function splitAtFirst.
@mmorella-dev
Copy link
Contributor Author

(okay, one more...)

@mmorella-dev
Copy link
Contributor Author

@amanharwara Build and tests passed!

Copy link
Member

@amanharwara amanharwara left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@amanharwara amanharwara merged commit c858e51 into standardnotes:main Jan 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants