search

dhleong / pepper-mint

Unofficial mint.com API in node.js
65 stars 16 forks source link

Use 'txnedit' task instead of 'simpleEdit' for updating transactions #14

Closed ScottG489 closed 6 years ago

ScottG489 commented 6 years ago

The 'txnedit' task is able to update all properties of a transaction including notes, tags, etc. It is also compatible with all fields which can be submitted with the 'simpleEdit' task.

Additionally I'd like to point out that with your args schema this should all be compatible. All of the field names are the same for both types of tasks. The 'simpleEdit' task seems to be used when you edit a transaction from the "Transactions" page on mint directly in the list of transactions. If you select a transaction in the transaction list and click on the "Edit details" tab just below the row, it expands the transaction and allows you to edit more details of the transaction. Clicking "Update" then posts the transaction update using the 'txnedit' task.

Let me know if you have any questions and I'd be happy to discuss this PR in more detail. I plan on updating the schema further to have explicit support for things like notes and tags if that's alright with you. However, for now I'd just like to make this small, nonbreaking change that will allow for future additions to the schema. Thanks!

dhleong commented 6 years ago

Hey, thanks for the PR!

I vaguely remember running into some issue trying using txnedit for my use case. Have you tested it to make sure omitting fields doesn't set them to blank, for example?

On Tue, May 8, 2018 at 2:48 AM Scott G notifications@github.com wrote:

The 'txnedit' task is able to update all properties of a transaction including notes, tags, etc. It is also compatible with all fields which can be submitted with the 'simpleEdit' task.

Additionally I'd like to point out that with your args schema this should all be compatible. All of the field names are the same for both types of tasks. The 'simpleEdit' task seems to be used when you edit a transaction from the "Transactions" page on mint directly in the list of transactions. If you select a transaction in the transaction list and click on the "Edit details" tab just below the row, it expands the transaction and allows you to edit more details of the transaction. Clicking "Update" then posts the transaction update using the 'txnedit' task.

Let me know if you have any questions and I'd be happy to discuss this PR in more detail. I also plan on updating the schema further to have explicit support for things like notes and tags if that's alright with you. However, for now these attributes can be added dynamically to the args parameter which will suit my immediate needs. Thanks!

You can view, comment on, or merge this pull request online at:

https://github.com/dhleong/pepper-mint/pull/14 Commit Summary

  • Use 'txnedit' task instead of 'simpleEdit' for updating transactions

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dhleong/pepper-mint/pull/14, or mute the thread https://github.com/notifications/unsubscribe-auth/AAx0FiJNkb49dSkRCX1wmbOBHO_NDqrmks5twT_AgaJpZM4T2GmK .

ScottG489 commented 6 years ago

I'm adding some tests to this PR which cover all of the supported fields. The currently supported fields exposed via args appear to be:

category catId date merchant txnId

My test proves all of those fields are still updated with the new task. Perhaps they changed their API since you last tested out the 'txnedit' task?

ScottG489 commented 6 years ago

Let me know if you have any other questions about my PR. I pushed a change addressing your concern about the process.exit() issue and responded to your other comments. Thanks!

dhleong commented 6 years ago

Sorry for the delay. The test and stuff looks good, I just had that one question I want to verify before merging this. Please let me know if my question is unclear!

ScottG489 commented 6 years ago

I just pushed a change that mostly addresses your concern about missing fields. I have a few things I'd like to expand on, however.

Firstly, the test which sends in undefined fields has been ignored. This is due to what appears to be a bug in PepperMint.editTransaction() which requires you to send in a date. I believe this is potentially a bug elsewhere as well since the root of the bug is that stringifyDate() always expects a date.

Second, you might notice that this doesn't prove that fields such as note, tag, etc won't be overwritten if not specified. As mentioned before, since these fields cannot yet be set with the current schema of args passed to editTransaction() then we're are safe. Once I update the schema of args (in a following PR) to allow for things such as note, tag, etc. then I will add those fields to the tests to prove they are unaffected if not specified as well.

Hopefully this addresses your concerns. Let me know what you think. Thanks!

dhleong commented 6 years ago

Firstly, the test which sends in undefined fields has been ignored. This is due to what appears to be a bug in PepperMint.editTransaction() which requires you to send in a date. I believe this is potentially a bug elsewhere as well since the root of the bug is that stringifyDate() always expects a date.

Well stringifyDate should probably enforce an input, but places where the date is actually optional could either handle that themselves, or we could introduce an stringifyOptionalDate which handles the omission. I'm no so much concerned about the date, however, since it is listed as a required field so any existing client code will be sending it. In this case, it makes more sense to re-enable that test, but don't remove .date, and update the assertions accordingly—but leave a note to add a test that does delete .date when and if that is supported.

Second, you might notice that this doesn't prove that fields such as note, tag, etc won't be overwritten if not specified. As mentioned before, since these fields cannot yet be set with the current schema of args passed to editTransaction() then we're are safe. Once I update the schema of args (in a following PR) to allow for things such as note, tag, etc. then I will add those fields to the tests to prove they are unaffected if not specified as well.

This is not really true, though. While I have tested and seen that it doesn't delete note, at least, the logic behind not verifying it isn't quite right. As you say, the schema doesn't support note in editTransaction, so it is impossible to provide it so no client will provide it. So we should verify that an existing note value—as you do set in the createTransaction—does not get deleted with the change from simpleEdit to txnEdit.

In other words, the existing method doesn't accept note, and the omission is currently known to not modify an existing value. This is desired behavior. Since we are changing the implementation, we have an obligation to verify that this desired behavior isn't lost. Since you already have these integration tests 90% of the way there, it'd be great to add this last little check in case future work modifies this method further.

You can do this simply by pulling out the "This is a test transaction" as a const at the top of the file, and asserting that the .note field is equal to it in both doAssertion* methods.

Thanks again for your hard work!

ScottG489 commented 6 years ago

Ok, I think I understand what you're saying. It doesn't matter that the user can't pass in something like a note. What matters is that when pepper-mint talks to the backing mint API it's not passing certain fields and this could possibly be construed by the API as them being "empty" and then it clearing those fields.

I've added an assertion that the note remains unchanged. Let me know if this is what you were looking for! I can add in assertions on other fields on top of just note to verify they haven't changed as well if you'd like.

dhleong commented 6 years ago

Okay great, thanks! Last are just a few style nits. I apologize that the .eslintrc files aren't up to date, but I don't see too many contributors to this project and rarely work on it myself these days.

ScottG489 commented 6 years ago

Pushed a few changes to address your comments.

dhleong commented 6 years ago

You rock, man 👍

ScottG489 commented 6 years ago

Thanks!