e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 32 forks source link

Download JSON Dump returning blank #994

Open Abby-Wheelis opened 11 months ago

Abby-Wheelis commented 11 months ago

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 commented 11 months ago

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

the-bay-kay commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 8 months ago

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):