Add import data #1333

theojarrus · 2021-02-25T20:25:39Z

Import feature

Added a function to import notes from json and plaintext

dmsnell · 2021-02-26T17:47:53Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+                                // note.setPublished(?);
+                                break;
+                        }
+                    }


the note data structure has a relatively fixed form. do we need to iterate over all the keys? I think we'll want all these fields and directly assigning them would be clearer and terser. that is, we don't really want to use a note if it doesn't contain content but this iteration+switch is happy to do just that.

note.setContent(noteJson.has("content") ? noteJson.getString("content") : ""); note.setCreationDate(noteJson.has("creationDate") ? DateTimeUtils.getDateCalendar(noteJson.getString("creationDate")) : now());

could be nice to move this into a new function such as noteFromJson(JSONValue json, List<String> trashedIds) and then it'd be easier to test in isolation as well.

First wrote it like this, but then found a solution with an iterator better. I'll change it! And where should I place the noteFromJson(JSONValue json, List<String> trashedIds) method? Ok if I leave it in a PreferencesFragment?

Good question. You might play around with it some and tell us what you think. I could see it having a good home in Note as well. That would bring it closer to the rest of the note semantics/models.

Good, thank you, I will think about it.

dmsnell · 2021-02-26T17:56:33Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+                    }
+                    note.save();
+                }
+            } else {


can we move these different decoders into their own methods? that will leave the top-level dispatch looking clear and the separate decoders more specific

String exportContents = readFile(uri); switch (getFileExtension(uri)) { case "json": return importJsonExport(exportContents); case "txt": return importPlaintextFile(exportContents); case "md": return importPlaintextFile(exportContents); default: Toast.makeText(…"unknown file format"…); }

after this change it'll be easier to isolate those exception domains vs. having this broad catch-all

Looks good, I will change this too

dmsnell · 2021-02-26T17:58:31Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+                Note note = noteBucket.newObject();
+                note.setContent(noteContent);
+                note.setCreationDate(Calendar.getInstance());
+                note.setModificationDate(note.getCreationDate());


I'll have to check, but I believe that we'll want to fill in default values, such as an empty list of tags and system tags

Ok, I thought that these fields are already filled by default when creating the note object, so I figured it might look like extra code. Should I add filling tags?

if they already get defaults it should be fine. looks like you beat me to confirming that 😉

😁 I'll check again just in case

dmsnell · 2021-02-26T18:02:25Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+            final MimeTypeMap mime = MimeTypeMap.getSingleton();
+            extension = mime.getExtensionFromMimeType(requireContext().getContentResolver().getType(uri));
+        } else {
+            extension = MimeTypeMap.getFileExtensionFromUrl(Uri.fromFile(new File(uri.getPath())).toString());


does Android do anything to more accurately estimate the MIME type from a document? if that's the case, if it does more than look at the extension, would it be more helpful if we used that vs. relying on the extension? or a combination of both?

Yes, I think I found a better solution, I'll fix it!

dmsnell · 2021-02-26T18:08:08Z

Simplenote/src/main/java/com/automattic/simplenote/utils/DateTimeUtils.java

@@ -37,4 +38,12 @@ public static String getDateTextString(Context context, Calendar calendar) {
        );
        return new SimpleDateFormat(pattern, Locale.getDefault()).format(calendar.getTime());
    }
+
+    public static Calendar getDateCalendar(String json) throws ParseException {
+        String pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";


this looks like an ISO8601 string. I see parseOffsetISO8601 in SDK 24 which I guess means we can't use that here. I looked briefly through androidx and didn't see one. do you know if there's an existing ISO8601 parser available in one of the compatibility or other libraries we already use?

Ok, I'll look for solutions!

dmsnell · 2021-02-26T18:11:59Z

@theo-jedi this is a great addition and I know people will love being able to import notes or accounts on their Android device. we're a bit backed up especially with Android so I apologize for our delays, which will be longer than usual. thanks for your contribution! I want to make sure we get you the proper attention on this so we can get it in, so please feel free to ping if we don't respond quickly enough.

theojarrus · 2021-02-26T18:17:38Z

Of course, I understand, thanks a lot, I will try to make corrections as soon as possible.

theojarrus · 2021-02-26T20:59:36Z

Now it looks much better, thank you! Unfortunately, I couldn't find better solution about ISO8601 string, but everything else seems to be fixed.

dmsnell · 2021-02-26T21:17:24Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+        }
+        for (int k = 0; k < notesArray.length(); k++) {
+            JSONObject noteJson = notesArray.getJSONObject(k);
+            noteFromJson(noteJson, trashedIds);


it would be preferable if this function returned a note (for testing and independence etc…). if we made that change this line would become

noteFromJson(noteJson, trashedIds).save();

Okay, I'll do it now

theojarrus · 2021-02-26T21:25:39Z

Done, thank you! 😀 Anything else?

dmsnell · 2021-02-26T23:20:15Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+        note.setTags(noteJson.has("tags") ? jsonToList(noteJson.getJSONArray("tags")) : new ArrayList<String>());
+        note.setPinned(noteJson.has("pinned") && noteJson.getBoolean("pinned"));
+        note.setMarkdownEnabled(noteJson.has("markdown") && noteJson.getBoolean("markdown"));
+        note.setDeleted(noteJson.has("id") && trashedIds.contains(noteJson.getString("id")));


not going to import any chords, tracks, or photos? 😉

😁 Thank you for your attention to my project! I really like Simplenote, that's why I took it as a basis. I use it every day, thank you very much for your work, I am very grateful to you!

So, what about merge? 😀

dmsnell · 2021-03-01T01:01:00Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+            Toast.makeText(requireContext(), getString(R.string.import_message_success), Toast.LENGTH_SHORT).show();
+        } catch (Exception e) {
+            Toast.makeText(requireContext(), getString(R.string.import_message_failure), Toast.LENGTH_SHORT).show();
+        }


this catch is uncomfortably broad. can we separate the exceptions from readFile from the exceptions from the importers? additionally, we have an AppLog that would be useful here for debugging issues that appear during the import. if you aren't sure how to add that we can do that. let's add a log when starting the import, indicating which type, and when it succeeds and fails. it would either fit as type ACTION or we might create a new type, IMPORT

I add exception handling. I think it's better to add the logs to you, because I don't know how to do it most correctly.

we'll take care of them, thanks

Good, thank you too!

dmsnell · 2021-03-01T01:01:53Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+        note.setMarkdownEnabled(noteJson.has("markdown") && noteJson.getBoolean("markdown"));
+        note.setDeleted(noteJson.has("id") && trashedIds.contains(noteJson.getString("id")));
+        return note;
+    }


looking at this now it does feel like it belongs in Note.java more than it does here. would you be willing to move it into that module and then import it? if now we can do that before merging.

Yes, this is much better, but I want to note that I move it there as a static method.

dmsnell · 2021-03-01T01:11:19Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+        note.setTags(noteJson.has("tags") ? jsonToList(noteJson.getJSONArray("tags")) : new ArrayList<String>());
+        note.setPinned(noteJson.has("pinned") && noteJson.getBoolean("pinned"));
+        note.setMarkdownEnabled(noteJson.has("markdown") && noteJson.getBoolean("markdown"));
+        note.setDeleted(noteJson.has("id") && trashedIds.contains(noteJson.getString("id")));


this is going to have some issues as I don't know that we can or should rely on imported notes containing an id. I believe that it's not really necessary anyway, more a byproduct of the way we've chosen to build an intermediate list of notes on import.

instead we could traverse each category of notes once and avoid needing to pass in List<String> trashedIds. there are a couple ways we can convey the trashed status but one easy way to do it would having to think about it much is to manually alter the JSON or the output note.

for (int i = 0; i < activeNotes.length(); i++) { noteFromJson(activeNotes.getJSONObject(i)).save(); } for (int i = 0; i < trashedNotes.length(); i++) { Note note = noteFromJson(trashedNotes.getJSONObject(i)); note.setDeleted(true); note.save(); }

(edit: corrected typo from notesArray to activeNotes/trashedNotes)

Yes, I had doubts about this piece of code, so much better, thanks!

dmsnell · 2021-03-01T01:16:19Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+            try {
+                list.add(json.getString(i));
+            } catch (JSONException e) {
+                e.printStackTrace();


we'll want to match our error logging with the rest of the app vs. dumping a stack trace.

in addition, it looks like the Note class stores its list of tags as a JSONArray so we could expose a new method for setTags(JSONArray tags) and skip the encode/decode loop.

on this point, are we able to directly instantiate a note with the existing Note class, without conversion? I doubt it works 100% but the constructor can take a JSON object. it may be the case that we can perform minimal transformation and/or slightly modify Note.java so we can go straight from the imported JSON object to a note.

new Note(activeNotes.getObject(i)).save();

I changed tags adding, but I doubt about the note. In theory, we can use the following code:

Simplenote currentApp = (Simplenote) activity.getApplication();
Bucket<Note> noteBucket = currentApp.getNotesBucket();
Note note = new Note("note - " + UUID.randomUUID(), activeNotes.getJSONObject(i));
noteBucket.add(note);
note.save();

It looks much better, but I doubt this is a good idea, since I am not sure how correctly the Ыimperium alogyrythms will work in this case. It seems to me that it is preferable to use noteBucket.newObject();

I'll leave this as it was for now, and then we'll decide!

thanks. I might need to consult with a few people on this. please pardon the delay, but expect a few days gap.

Sure, no problem!

dmsnell · 2021-03-01T01:21:31Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+
+    public String getFileExtension(Uri uri) {
+        return MimeTypeMap.getSingleton().getExtensionFromMimeType(requireContext().getContentResolver().getType(uri));
+    }


these two might be great in a FileUtils class in the utils folder.

Yes! Wanted to do it but was afraid to create new files.

all good. I know that fear, especially entering an open-source repo for new contributions.

we'll let you know if we want something to change, or we'll just end up changing it on merge.

Cool, thank you! 😀

dmsnell · 2021-03-01T01:23:13Z

Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java

+    private void importData(Uri uri) {
+        try {
+            String exportContents = readFile(uri);
+            switch (getFileExtension(uri)) {


I tried confirming the return values for getType(uri) and didn't see anything helpful. did you see a list? I saw that it does chop off the first part of the mime type.

we probably want to add unit tests to cover this function for the expected file types we will support importing. can it really suggest md as the extension? I guess if it detects plaintext and has .md?

this is a nice update from before though. I like that there's some leeway if someone has a file with a missing or wrong file extension.

Maybe I misunderstood, but I guess it works a little differently. It seems to me that it makes no difference to us what extension the file is, except for json, of course. All the rest will definitely be text (this is exactly what mimes are for), because I set mime types application/json and text/*, which includes txt, md, html and other, but there's no difference for us between them.

Here's a list (section "common examples"): en.wikipedia.org/wiki/Media_type

my point was that I don't see how it's possible here to ever match on "md", unless Android is making a guess on that which I don't think it would?

there is a distinction that's possible, though we could handle all text/plain files the same way, but that distinction is whether or not we toggle markdown support in the note. if we happen to know it's a .md file and it's plaintext then why not go ahead and import it with markdown enabled. otherwise, if it's plaintext import it just as raw text.

Ah, I understand, I apologize for what you had to explain. However, unfortunately, I still did not understand what the problem was with the written code, I added the method getFileExtension(Context context, Uri uri) that gets the file extension from uri. getType(uri) returns not an extension, but a mimetype, which I'm using for getExtensionFromMimeType(mimeType)

here's the central question: can getExtensionFromMimeType(mimeType) ever return "md"?

the code is written as if it can, but I want to make sure that's the case.

Yes, of course I checked!

Well, should I add a parameter boolean isMarkdownEnabled to importPlaintextFile(String exportContents) method?

yeah I think so, or possibly better a string or enum WithMarkdown | WithoutMarkdown to avoid dangling and curious true values. I don't foresee any other options we'd want to pass in there such as tags or share urls so it should be good to introduce this one flag

Good, did! It seemed to me, that enum will be better than a string

dmsnell

Thanks for your continued work and patience. I ended up leaving more comments than I anticipated, but there were a few things I wanted to make sure we covered. We will also be performing testing on this as well as the merge. I don't know that it's totally ready for that now, but it looks fairly solid and after a few details are ironed out I think we'll be good.

theojarrus · 2021-03-01T15:23:37Z

Sure! Thank you, understood, I'll do it!

theojarrus · 2021-03-01T18:04:05Z

Pushed the current changes, I'm waiting for decisions on other issues 😃

theojarrus · 2021-03-01T18:53:43Z

Thank you very much for your work again!

theojarrus · 2021-03-04T13:17:24Z

Good afternoon, is there any news? 😀

dmsnell · 2021-03-04T17:01:04Z

@theo-jedi no news, apologies. will try and check in again next week. we haven't forgotten about you though.

theojarrus · 2021-03-04T18:34:40Z

All good, don't worry, I understand, I'll wait, thank you!

dmsnell · 2021-03-19T22:32:36Z

Simplenote/src/main/java/com/automattic/simplenote/Importer.java

+        JSONArray trashedNotes = export.optJSONArray("trashedNotes");
+
+        for (int i = 0; activeNotes != null && i < activeNotes.length(); i++) {
+            addNote(activeNotes.getJSONObject(i));


sorry for the error in my branch. by the way, won't the error still occur here on account of the fact that the export format is different than the Simperium data model? we can't shove in invalid data…

for example, the lines loading the time formats were there to convert to the internal representations (unless it was those representations that were wrong, as I know that Simplenote stores the data as seconds-since-epoch).

I like simplifying the code but I don't think it's a good direction to skip parsing - we'll just be asking for data corruption and crashes.

Yes, of course I understand... The problem is that this method simply creates an object with the given parameters. We expect the markdown field to be a boolean, but the user can replace it with a string and this will crash the application. Now I'll try to roll back the changes

theojarrus · 2021-03-21T16:33:04Z

Something like that. It works pretty well for me, what do you think? Do we need to do something else? Maybe it's time to add an AppLog?

peril-automattic · 2021-03-22T23:27:19Z

You can test the changes on this Pull Request by downloading the APK here.

dmsnell · 2021-03-23T00:38:24Z

Yeah @theo-jedi the code is looking good. I wish we didn't have to send the notes bucket into Note.fromExportedJson and the other, but I think it's something tolerable for now.

At this point most of the finishing work will likely be on our side. I'll try and figure out best how to do that since I haven't done it before, whether that's us merging onto this branch or merging this into one of ours.

theojarrus · 2021-03-23T18:08:47Z

Great, thanks for the experience, it was a pleasure working with you! 😊

I suggest to send note object into Note.fromContent and Note.fromExportedJson instead of bucket, and then change call in Importer to Note.fromContent(mNotesBucket.newObject(), content). I can do this, I just thought that it is better not to make new edits in case you started the final preparation.

Hope everything goes well, thank you so much again, keep in touch.

dmsnell · 2021-03-23T21:07:38Z

I can do this, I just thought that it is better not to make new edits in case you started the final preparation.

that's something I considered as well, but then it seemed to be even more confusing of an interface than passing in the bucket itself. I think this butts up against the way this app interacts with the Simperium library and so I don't think we need to get hung up here in this PR on it. thanks for being open to change and feedback though!

Great, thanks for the experience, it was a pleasure working with you! 😊

Same back to you!

theojarrus · 2021-03-24T05:13:06Z

Okay, got it! Thank you again!

SylvesterWilmott · 2021-03-24T13:18:33Z

This all looks great, thanks @theo-jedi. UI is perfectly done.

We're introducing new messaging around imports and failed imports, currently as toast messages.

If I can make a couple of suggestions, I think we could make the messages more user-friendly:

Data loaded > Import successful

Our general error copy convention is "Cannot [action]":

Parse error > Cannot parse file
File error > Cannot read file

Hopefully the above accurately represent what the error actually is, if not feel free to suggest something. Could you also clarify what would trigger Date error?

In my opinion - yes. I checked that when adding a note to the trash, the tags remain (until they are removed from the tags menu), so I think we shouldn't lose this information.

Echoing @sandymcfadden. Tags should be kept for trashed tags.

theojarrus · 2021-03-24T14:17:01Z

Thank you so much! 😊

If I can make a couple of suggestions, I think we could make the messages more user-friendly. Hopefully the above accurately represent what the error actually is, if not feel free to suggest something

Yes, of course, I'll do it now!

Could you also clarify what would trigger Date error.

Oh sorry, I forgot to remove this line, it was previously used instead of Parse error, now it is no longer needed.

Echoing @sandymcfadden. Tags should be kept for trashed tags.

Yes, @dmsnell and I came to the conclusion that the tags should be keeped for trashed notes, that's how it is now

theojarrus · 2021-03-29T16:44:33Z

Good evening, let me know when you're ready to merge. If I need to make any other final edits (or any actions with git), I'm here 😉

theojarrus · 2021-04-05T15:54:03Z

Good afternoon, have any news?

dmsnell · 2021-04-06T23:35:24Z

good afternoon, have any news?

unfortunately not. I'm doing too many things simultaneously. this is a great contribution; it's all on us to get it moving faster. thanks for your patience.

SylvesterWilmott · 2021-04-07T11:52:08Z

Green light from me design-wise. Thanks for the updates @theo-jedi

theojarrus · 2021-04-07T13:08:05Z

unfortunately not. I'm doing too many things simultaneously. this is a great contribution; it's all on us to get it moving faster. thanks for your patience.

Sure, all good! I just got worried because there was no response to the latest changes. Sorry for the unnecessary worry, I will wait, good luck with your work, and thank you!

theojarrus · 2021-04-07T13:10:06Z

Green light from me design-wise. Thanks for the updates @theo-jedi

Great. Thank you for your attention to this code! 😊

dmsnell · 2021-04-15T20:56:12Z

Status update for you @theo-jedi:

this is really good work and we want to merge it
our development is paused temporarily as we shuffle some things on the Android app
we won't forget this PR but we shouldn't expect any movement soon
when we do merge it we need to add the AppLog entries (that's on us at Automattic)
thank you for your patience and diligence; we're going to stress that a bit more but will update you when we resume work on this

theojarrus · 2021-07-06T14:01:19Z

Hey, good day @dmsnell! What about working on the merge of this pull request? Is it still not available?

danilo04

Hi @theo-jedi, I am going to merge this PR. Thank you for the implementation of this feature. I will be adding and checking a few details to make sure the feature is complete before release. I have created issue #1417 to keep track of the changes that need to be added.

theojarrus · 2021-07-16T23:29:28Z

Great, thank you so much @danilo04! I was really glad to help, very nice! 😊

Add import data

61f44af

dmsnell reviewed Feb 26, 2021

View reviewed changes

Import changes

f0ccfa1

dmsnell reviewed Feb 26, 2021

View reviewed changes

Changing noteFromJson to return Note

21c6bdf

dmsnell reviewed Feb 26, 2021

View reviewed changes

dmsnell reviewed Mar 1, 2021

View reviewed changes

Import changes

c5e2bdd

Import markdown check

4857d46

dmsnell added the [Type] Enhancement Improve existing functionality. label Mar 2, 2021

dmsnell reviewed Mar 19, 2021

View reviewed changes

fedor-jedi added 2 commits March 20, 2021 17:47

Revert "Importer changes"

ade7ae0

Added import file validation

0ea324a

dmsnell mentioned this pull request Mar 22, 2021

Add/import on pr 1333 #1350

Closed

Change import error messages

7d93384

danilo04 self-requested a review July 16, 2021 19:05

danilo04 assigned theojarrus Jul 16, 2021

danilo04 mentioned this pull request Jul 16, 2021

Final details for Import Data Feature #1417

Closed

4 tasks

danilo04 approved these changes Jul 16, 2021

View reviewed changes

danilo04 merged commit 390cbc8 into Automattic:develop Jul 16, 2021

theojarrus deleted the import-feature branch July 16, 2021 23:29

danilo04 mentioned this pull request Jul 19, 2021

Add final details to Import Data Feature #1418

Merged

danilo04 added this to the 3.0 milestone Jul 26, 2021

Add import data #1333

Add import data #1333

Conversation

theojarrus commented Feb 25, 2021

Import feature

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Feb 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Feb 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Feb 26, 2021

theojarrus commented Feb 26, 2021

theojarrus commented Feb 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theojarrus commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Mar 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

theojarrus commented Mar 1, 2021

theojarrus commented Mar 1, 2021

theojarrus commented Mar 1, 2021

theojarrus commented Mar 4, 2021

dmsnell commented Mar 4, 2021

theojarrus commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theojarrus commented Mar 21, 2021

peril-automattic bot commented Mar 22, 2021

dmsnell commented Mar 23, 2021

theojarrus commented Mar 23, 2021 • edited

dmsnell commented Mar 23, 2021

theojarrus commented Mar 24, 2021

SylvesterWilmott commented Mar 24, 2021

theojarrus commented Mar 24, 2021 • edited

theojarrus commented Mar 29, 2021

theojarrus commented Apr 5, 2021

dmsnell commented Apr 6, 2021

SylvesterWilmott commented Apr 7, 2021

dmsnell Feb 26, 2021 •

edited

dmsnell Feb 26, 2021 •

edited

theojarrus commented Feb 26, 2021 •

edited

dmsnell Mar 1, 2021 •

edited

theojarrus commented Mar 23, 2021 •

edited

theojarrus commented Mar 24, 2021 •

edited