-
Notifications
You must be signed in to change notification settings - Fork 540
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
Export Transactions: previously imported transactions are exported forever #467
Comments
@rivaldi8 this sounds similar to some report we got a while back right? |
I've been trying, but I haven't been able to reproduce. I've tested both with the attached example file and with my own files. It never exports the imported transactions, only the newly entered. I'm not sure what might be provoking this. Have you changed any setting? In my case I've done the tests from a bq Aquaris E4 (Android 5.0). |
@rivaldi8 I wonder too, because I can't reproduce it. But a small subset of users seem to be having the issue. I haven't changed any settings with this feature. Either there is a disconnect between what the users expect and what the feature actually does, or there is a very subtle bug that we will hopefully find one day. |
Hi guys! I think we are facing another time zone issue. My time zone is I changed my device to a non negative time zone (I used |
@alceurneto thanks for that additional info. That helps in fixing the problem |
Hi guys, I've digged a little the source code in hope of better understanding of this issue. I've noticed the trigger responsible for updates on database column static String createUpdatedAtTrigger(String tableName){
return "CREATE TRIGGER update_time_trigger "
+ " AFTER UPDATE ON " + tableName + " FOR EACH ROW"
+ " BEGIN " + "UPDATE " + tableName
+ " SET " + CommonColumns.COLUMN_MODIFIED_AT + " = CURRENT_TIMESTAMP"
+ " WHERE OLD." + CommonColumns.COLUMN_UID + " = NEW." + CommonColumns.COLUMN_UID + ";"
+ " END;";
} and the On a device with timezone
I've struggled a lot to spot the difference between Of course the basecode is all new to me and I must be missing something. P.S.1: I've found this StackOverflow topic about the use of P.S.2: Because the mobile device is subject of sudden (and automated) change of timezone, I'm favour of attributes like |
Hi guys, I think I've understood the issue. As soon as I've realized that our When importing transactions the The issue arises when we insert a new transaction in the mobile app. For example, @Override
protected @NonNull SQLiteStatement setBindings(@NonNull SQLiteStatement stmt, @NonNull Transaction transaction) {
stmt.clearBindings();
// ...
stmt.bindString(7, transaction.getCreatedTimestamp().toString());
// ...
return stmt;
}
The same occurs for public void addRecord(@NonNull final Split split, UpdateMethod updateMethod){
// ...
//modifying a split means modifying the accompanying transaction as well
updateRecord(TransactionEntry.TABLE_NAME, transactionId,
TransactionEntry.COLUMN_MODIFIED_AT, new Timestamp(System.currentTimeMillis()).toString());
} Our last export time has analogous issue. For example, @Override
public List<String> generateExport() throws ExporterException {
// ...
/// export successful
String timeStamp = new Timestamp(System.currentTimeMillis()).toString();
PreferenceManager.getDefaultSharedPreferences(mContext).edit().putString(Exporter.PREF_LAST_EXPORT_TIME, timeStamp).apply();
// ...
} We need evaluate if there is the same issue with other model entities and exporters. I'm capable to make a try and work to fix this problem but I need some guidance:
|
@alceurneto I think your analysis makes A LOT of sense. Thanks! One way I would suggest to fix this, is to work with I should really get around to writing more in the wiki about contributing. But basically, contributions should go into the |
@codinguser Thanks! I'll work on it ASAP. |
One thing to think about would be the I have to think about it a little more. |
That's correct! We need to think about the transition from the old (some transactions in local time) and the new (all transactions in UTC) regarding existing transactions in database. |
I've started working to fix this issue. |
Hi guys, Some thougts here: If we don't update For users with time zone equals UTC - NOn this picture we can see a time line in UTC. At the 'now' moment, or UTC + 0, occurs a GnuCash Android (GCA) update fixing the current issue without updating our fields. It's pictured in yellow. When we have an import (export is analogous) occurred before the version update, the The same occurs for transactions added or edited in the old version. The Added or edited data in the new version has equal time occurrence and This states that, despite the issue has been fixed, the first export operation will have the same wrong result that we see nowadays. After this first attempt (or after an import) I believe the exports will have correct results. For users with time zone equals UTC + 0They fortunately have no problems. For users with time zone equals UTC + NAs in previous case we can see the time occurrence for GCA update, but now In the future are too That is why users without negative time zones have not noticed the issue. On the other hand, after the update, all transactions added or edited in the new version will be NOT eligible for export IF they are entered at least N hours after the last import (if they are entered (N+m) hours after last import time, they will be eligible). Differences between string format of
|
Thanks for the input @alceurneto If a user local time is UTC+N, then the transactions have a time of UTC which is behind A few more thoughts about fixing this bug:
Of course, this course of action means that we would probably break the next export after the update for some people. But I guess it's just the growing pains. If we go about modifying the database, I fear we may do more damage for more users. What do you think? |
Hi @codinguser, Thanks for replying!
My NOT native english may be doing the things a little harder ;-)
Until the user do an export at least UTC. An import resets the problem in the current version.
The added/edited new & modified_at pictured in green are operations executed in the new version of GCA (with our fix).
If we do that for users with UTC - N the transactions pictured in blue will be NOT eligible for export.
I agree! Now guys, some new questions:
|
The question is, how do you changed the Either way, we need to fix the code to handle all times in UTC. This might result in some export duplicates or skipped transactions. (But we usually have backups and data is rarely lost even if user deletes). During export, users can just adjust the time in the form to get all their transactions out. After the initial hiccups for a few UTC-N users, it will all be fine. Right now, I'm open to any ideas how we can fix the code going forward and also fix existing timestamps for users. I do not think it is possible to do both for all users, but I may be wrong.
The
I think this fix should go into the |
I agree with fixing the code to use UTC and to not try to fix wrong timestamps in the database. Then, the first export after the application update should select transactions where However, there might be something I haven't understood, because some numbers don't seem to add up in my tests. If I do an import at 7:00 UTC+1 it's stored as local time in Next, I do an export where Moreover, the transactions added just after the import are exported correctly (I understand they shouldn't). The imported ones aren't exported, as expected. |
Hi guys, Thanks for helping.
Ops, my bad! It's there all the time...
No problems. I'll do it but will I create
I'll do my PR in |
@rivaldi8
@alceurneto you're right, it should be in v12. Also make sure to update the DATABASE_VERSION in Also just thought to mention that the app already has a dependence on the Joda time library which is great for time manipulation. In case you need it. Thanks for the attention to this issue. |
Hi @codinguser !
I'm checking if I've understood all the things: we will covert |
@alceurneto that's at least what I understood from you and @rivaldi8 analysis of the situation. But I must say you understand this bug better than I do. Do you recommend a different course of action? |
@codinguser Thanks for replying. I'll think a little more about it. |
@alceurneto sorry about the confusion. As @codinguser points out, It seems I misunderstood what was happening with the time conversions, so my proposed solution for the first export might not be appropriate. You'd better solve it in the way you find more suitable :) I'll try to help if I find some time to look into it. |
@rivaldi8 no problem. There are many variables here and we are looking for thorough understanding together. |
@alceurneto Because of the different timezones, we cannot find a one-size-fits-all solution. But I thought of a much simpler solution. What do you think? |
Would be an elegant solution. The same warning message could be sent to all users. Do you know what is the field used for GnuCash desktop to detect duplicates during imports? |
I think GnuCash desktop uses a combination of heuristics. But I can also imagine that as the transaction ID would be the same, it could easily use that to detect duplicates. All transactions have unique IDs. |
Hi guys! I have almost finished and I'm working now to set last-export-time to UTC - N for everyone. I'm coding All is OK but I need a I've seen that this method is called by reflection and I'll make the changes with care if you agree. Thanks in advance. |
You can get a context anywhere using No need to change the method signature |
That's right! Thanks! |
Hello guys! Please check the PR is OK. Strictly speaking our update in version migration shifts last-export-time N hours earlier on time, where N is the absolute value of user time zone offset for all users. Some values extracted from my test logs when running User time zone GMT-05:00 Eastern Standard Time: User time zone GMT+05:00 Pakistan Standard Time: If you have doubts please comment. |
It looks alright to me, I've just made some minor comments inline. I've also made some (non-exhaustive) tests and it seems to be working fine. |
Thanks @rivaldi8. |
@alceurneto I've had a quick look and it looks alright. Thanks for your work on this issue. 👍 I thought just the |
@codinguser Thanks.
Yes. They are covered. I did some additional tests importing from GnuCash desktop (GCD) and importing to it again. The Keep in mind that transactions entered on GCD receives arbitrary 00:00:00 local time: <gnc:transaction version="2.0.0">
...
<trn:date-posted>
<ts:date>2016-01-31 00:00:00 -0200</ts:date>
</trn:date-posted>
<trn:date-entered>
<ts:date>2016-02-06 17:57:30 -0200</ts:date>
</trn:date-entered>
<trn:description>Gas</trn:description>
...
</gnc:transaction> When desktop and mobile have not the same time zone we could see things like:
And nothing is wrong here. I did a little research and seems GCD has a time field but its not used (keeping the zero value). When the desktop application receives a time zone change the data-posted could change in a weird, but not wrong, way (https://bugzilla.gnome.org/show_bug.cgi?id=137017). |
@codinguser I think I've missed a test with OFX format. Sorry! I'll do it ASAP. |
@alceurneto awesome 👍 |
I think there are no problems with OFX export.
public final static SimpleDateFormat OFX_DATE_FORMATTER = new SimpleDateFormat("yyyyMMddHHmmss", Locale.US); But adds a suffix with time zone information this way: public static String getOfxFormattedTime(long milliseconds){
Date date = new Date(milliseconds);
String dateString = OFX_DATE_FORMATTER.format(date);
TimeZone tz = Calendar.getInstance().getTimeZone();
int offset = tz.getRawOffset();
int hours = (int) (( offset / (1000*60*60)) % 24);
String sign = offset > 0 ? "+" : "";
return dateString + "[" + sign + hours + ":" + tz.getDisplayName(false, TimeZone.SHORT, Locale.getDefault()) + "]";
} The exported file contains: <STMTTRN>
<TRNTYPE>CREDIT</TRNTYPE>
<DTPOSTED>20160207224456[-3:GMT-03:00]</DTPOSTED>
<DTUSER>20160207224456[-3:GMT-03:00]</DTUSER>
<TRNAMT>109.00</TRNAMT>
<FITID>cbe5ace62bc641bfafbe57a2d069a587</FITID>
<NAME>Gas</NAME>
<BANKACCTTO>
<BANKID>org.gnucash.android</BANKID>
<ACCTID>3334f560c5dce6205671234a58a48094</ACCTID>
<ACCTTYPE>SAVINGS</ACCTTYPE>
</BANKACCTTO>
</STMTTRN> When I imported the file on GnuCash desktop (GCD) the result didn't make sense to me but it's outside the scope this issue. The new transaction date was correct but the time is always a zero value (as I could see after an export from GCD). Finally, one last question: is OFX the standard format used to exchange data between mobile and desktop? |
That's correct, OFX includes timezone info. So I guess we need not worry about that. I will merge the pull request and make a beta release available so more people will test. |
Perfect! I'm thrilled to have helped the team. |
We'd like to continue to have your help ;) |
How to reproduce
If we enter another transaction in GCA and export it again the generated QIF file will have the same previously imported transactions plus the new one.
P.S: We cannot import the QIF file in GnuCash desktop as stated at #466
Additional information
Sample files
sample-files.zip
The text was updated successfully, but these errors were encountered: