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

Download JSON Dump returning blank #994

Open
Abby-Wheelis opened this issue Oct 2, 2023 · 7 comments
Open

Download JSON Dump returning blank #994

Abby-Wheelis opened this issue Oct 2, 2023 · 7 comments

Comments

@Abby-Wheelis
Copy link
Member

Currently, the "download json dump" feature is not working. When the button is clicked and the file is emailed, the file is just an empty array. We need this feature to help with debugging and looking at timeline data.

This was recently re-factored to use React-Native-Paper's date picker to choose the date to download data. This could have been when the break occurs. The Date is chosen, then ControlHelper.getMyData(date) is called. One possible issue is that the date format is different than what the ControlHelper expects, but I will have to investigate further to know what is going on for sure.

@shankari
Copy link
Contributor

shankari commented Oct 2, 2023

I believe @the-bay-kay is looking into this

@the-bay-kay
Copy link

Yes! I've got a small change left to wrap up issue #979, but was looking into this this morning, and will keep digging shortly!

@the-bay-kay
Copy link

Quick update on where I'm at debugging:

I think the issue may be the call to getRawEnteries in the ControlHelper service, like you suggested. By the time writeDumpFile is called, result.phone_data is already empty. I need to read up a bit on cordova's pushGetJSON and the way moment objects are processed, just to better understand how we need the message object to be constructed. Line 172 currently outputs the message object as the following:

[0] [phonegap] [console.log] About to return message {"key_list":null,"start_time":1696143599,"end_time":1696143599,"key_time":"metadata.write_ts"}

@the-bay-kay
Copy link

the-bay-kay commented Oct 3, 2023

It seems that the getRawEnteries is really only given date info by the JSON dump. I'm still hunting for which route is called by the pushGetJSON (Is it request_data() in e-mission-server/emission/public/pull_and_load_public_data.py? Looking into that)

Since I haven't been able to compare it to the date-data used by the other GET methods, I've been checking out the commit history. After looking at how the previous date picker was written, the one thing that stands out to me is the input format. The previous datepickerObject had dateFormat: 'dd MMM yyyy', : Was the extra M intentional? The docs for ion-datetime has one code snippet with a third M, suggesting there may be a difference in how the data gets recorded. The current react DateDatePicker doesn't use this format, and ControlHelper has always expected 'YYYY-MM-DD'.

I tried to revert to 13678e0 to check the old format of the data being passed, but ran into an issue with a cordova plugin loading. There's a real possibility I'm barking up the wrong tree, but I'd like to cross this off the list of possibilities. I'll try seeing if we can formated the DateDatePicker data in a similar way to the older version.

(edit: DatePickerModal doesn't seem to format like the ion component!)

@the-bay-kay
Copy link

the-bay-kay commented Oct 3, 2023

I think I've got it! Thank you for pointing me in the right direction Abby, you were right - the issue was that ControlHelper needed the date to be in the dd MMM yyyy format. I had issues formatting in the DataDatePicker component, but was able to get it working with services.js. I'm going to go ahead and do a few more tests on Android to confirm this fix worked before making a PR. (Edit: The download works! Making the PR first thing tomorrow morning, I've attached the test screenshots below.)

I did have a question about the use of the luxon library. Jack and I discussed today how the moments library has shifted itself to being a 'legacy codebase', and he's mentioned before that we may want to think about switching entirely to luxon. This is probably a pretty low priority change, but should I go ahead an open an issue so that we at least have it on the docket? Or, since I'm already using it in the file, should I go ahead and update all of services.js so it's fixed in one PR?

image
image(1)

@JGreenlee
Copy link

Can you start a draft PR so I can see how you got it working?

There are actually 2 files called services.js (one in /js and one in /js/diary). Both of them still use moment in some places, and I was planning that we would convert them to using Luxon while we are getting those files rewritten.

Making incremental tweaks to the .js files at this point is not super efficient. If we can fix things up in the process of rewriting, we'll save time.
So I suggest taking this opportunity to rewrite ControlHelper into a new TS file (controlHelper.ts or something). And in the rewritten version, use Luxon.

@the-bay-kay
Copy link

Just wanted to give some post-mortem documentation on this issue!
The JSON dump feature was fixed in PR 1052 , and merged into service-rewrite-2023. Once those changes are merged into production, we can reevaluate this issue, Issue #994, and #936. I did want to make a couple notes on the state of the JSONDump, while I'm at it (Some "future work" notes):

  • As can be seen in the PR, there are some hiccups with the timeline downloading / uploading process.
  • The trip download feature is working again; from visual inspection of the files and repeated testing, this function is now working again on a variety of opcodes / devices/ OSs
  • When loading trips onto the dev server, certain days fail to load. This is documented further in the PR - we determined this was a regression (See "Download json dump" causes crash for some days #936).
  • In the future, I think there are a few places we could start to fix this issue:
    • Do a closer comparison of the working / broken timeline files. The issue may be in how certain days are recorded, organized, etc.
    • Look into the server intake process -- the exact point of failure is outlined in 1052. Perhaps the data collected is correct, but there are some issues with the intake process!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Issues being worked on
Development

No branches or pull requests

4 participants