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

upload service rewrite #988

Closed
Abby-Wheelis opened this issue Sep 27, 2023 · 14 comments
Closed

upload service rewrite #988

Abby-Wheelis opened this issue Sep 27, 2023 · 14 comments

Comments

@Abby-Wheelis
Copy link
Member

Upload Service -- Child Issue of #977

One of the angular services that needs to be rewritten is uploadService.js.

This service has one exported function, used in one place ProfileSettings for uploading the logs. The other 5 functions are helpers for uploadFile. However, this function is fairly complex, makes extensive use of promises, and has several instances of ionicPopup.show and ionicPopup.alert that need to be replaced somehow

@Abby-Wheelis
Copy link
Member Author

I went to test this on my test phones, to see what the flow of popups should look like, and could not find the feature. As it turns out, this is enabled by one of the variables in the config file, which is only true in dev-emulator-study, dev-emulator-program, and dev-emulator-timeuse. Meaning the feature is not in production, or in staging, for that matter, at all right now. I took a peek at the blame, and it looks like this feature was added ~2 years ago and was a potential replacement for emailing logs. @shankari is this still the case?

I can see where this is a valuable tool for debugging from a user experiencing issues, but am struggling to run the existing flow in the emulator -- maybe having to do with uploadConfig.json.sample, so I might need some guidance on the best way to test this function.

@shankari
Copy link
Contributor

@Abby-Wheelis I added this because people were having issues emailing the logs - if the log file is too large, then it cannot be attached in most email clients. There were also issues with people using non-default email apps (e.g. gmail on iOS) so that their default app was not even set up and could not email logs.

However, it requires a new REST API on the server side (https://github.com/e-mission/em-bug-upload) to accept the logs, and I have not yet applied for cyber permission for that service, so we cannot host it in the NREL environment.

So to test this, you do in fact have to:

  • use a dev-emulator config that does not have the upload disabled
  • run a local copy of em-bug-upload AND
  • change uploadConfig.json.sample (or copy it to uploadConfig.json) and point it to your local copy of em-bug-upload

If anybody wants to volunteer to do get cyber approval for the upload service so that we can re-enable this feature, I would be very supportive!

@Abby-Wheelis
Copy link
Member Author

I added this because people were having issues emailing the logs

That makes a lot of sense, I believe it would also then resolve the thought we were having about being able to email more than one day of logs (which we can't do right now because of size limitations).

With the testing steps you've given, I can hopefully get it working locally, and therefore be able to test as I convert the service, so that we have it ready if/when we get approval and have the server side set up.

@Abby-Wheelis
Copy link
Member Author

I was able to get things working with the em-bug-upload locally, so moving forward with trying to rewrite and test.
There are quite a few ionic popups in this helper, that I'm working to brainstorm replacements / substitutes for. Some of them can be handled by the displayError from logger, but there's two more "interactive" popups that might need some tweaking:

  1. a "please enter details" popup that asks users to put in what is wrong, while showing them where the upload is going. I was thinking that we could move this popup to the profile screen, and then pass in the input we received when we call the upload function in the helper. Any objections to this?
  2. There is a "loading" popup that shoes when the user starts the upload that has a "spinner" and a cancel option - I'm not sure how to do this with the sorts of "simple" popups that we can create here. I was thinking we could just notify the users that it's started, then when it ends (currently we have loading and end) but that does not provide a cancel option.

@Abby-Wheelis
Copy link
Member Author

As a status update (and reminder to my Monday self) -- the issue I was having with a hook out-of-place was because I was using useTranslation in the service. I've resolved that issue for now, but am still working on having the service functioning in the emulator, so I can start trying to implement comprehensive unit tests.

@Abby-Wheelis
Copy link
Member Author

Right now, I'm stuck on configuring the post request in a way that doesn't throw an error. From what I'm getting back from the debug server right now, the request is set up in such a way that the "reason" parameter is not properly found. The previous request is:

const sendToServer = function upload(url, binArray, params) {
    var config = {
         headers: {'Content-Type': undefined },
         transformRequest: angular.identity,
         params: params
     };
     return $http.post(url, binArray, config);
}

and the new request is looking something like:

const sendToServer = function upload(url, binArray, params) {
    var config = {
        method: "POST",
        body: JSON.stringify({
            data: binArray,
            params: params
        }),
        headers: {'Content-Type': undefined },
    };
    return fetch(url, config);
}

but I continue to get errors about "reason" (one of the params) being undefined. I'll keep working at it, but I haven't been able to get past the error yet.

I've been referring to docs on fetch and other information about passing information to the server here.

@Abby-Wheelis
Copy link
Member Author

I got a .ts version of the upload service working in the emulator yesterday. Today, I've been slowly piecing together enough mocks (fetch, fetch for post, file, fileSystem... ) to get a test case to run. I hope to be able to wrap up a working test tomorrow.

This isn't going to be perfect - we will definitely want to revisit the UI to restore a spinner, for example, but it will get us away from angular for now, and make sure the functionality is tested enough that we it won't be broken when we're ready to resurrect the functionality.

@Abby-Wheelis
Copy link
Member Author

I now have a working test for this as well as having it working in the emulator. I think the last thing I want to do on this pass is mock window.alert to keep some notification of starting and success on the upload, but may leave better UI notifications to the next pass on this.

@Abby-Wheelis
Copy link
Member Author

I mocked window.alert this morning and have the tests working, but now I'm noticing a regression in using it in the app itself. When I go to upload to the development (em-debug) server, the older files from when I was using the angular version have WAY more contents.

Something has gone wrong I believe with the way that I'm sending the binString variable into the POST request. I had to change the formatting slightly going from $http.post to fetch and in that process found it necessary to JSON.stringify the body of the request. But now the "data" is just an empty object and not the large file the angular version was sending up, so I need to keep working this out.

@Abby-Wheelis
Copy link
Member Author

Abby-Wheelis commented Oct 5, 2023

I found this related issue: #609 which details the process from the first time this was implemented.

It is clear that sending the DataView to the server was an intentional choice. However, the difference between $http.post and fetch meant that I completely changed the way things were sent up to the server.

I need to find a better way to send the file to the server, but I'm not really sure what that looks like right now. I tried briefly to read the whole ArrayBuffer into a String, but that errored out because it was too big. The fetch version of POST seems to really recommend that we send a string, or a stringified JSON object.

I just tried reverting to only uploading the file, and hardcoded the file name - which worked to upload the contents, but of course lost the "reason" and the "timezone". This fetch request works to upload the contents, in their SQLite fornat.

fetch(url, {
        method: 'POST',
        headers: { 'Content-Type': undefined },
        body: binArray
    } )

Now, I just need to get the other data parameters back into the request, back out of it at the server, and make sure nothing in the tests broke!

@Abby-Wheelis
Copy link
Member Author

Abby-Wheelis commented Oct 6, 2023

Reading through the previous thread again, and seeing that FormData (the recommended route most places I've read) did not work, when I tried I got the same raw output as @shankari did last time.

I did, however, notice that she used "url encoding" and worked (that's what the third parameter in the old post was, I did not realize). So next thing I'll try is embedding these params into the url myself, since fetch won't take a 3rd parameter. Hoping this restores functionality 🤞

@Abby-Wheelis
Copy link
Member Author

Abby-Wheelis commented Oct 6, 2023

Adding the url encoding manually worked! This is what I should have done in the first place, I just didn't realize it. The reason is now going through and being read, and I just need to include the timezone properly to fully restore function.

@Abby-Wheelis
Copy link
Member Author

Upload Service is now in React, working in the emulator, and has a test it passes. I had originally needed to alter the em-bug-upload code with how I changed the post request, but when I switched back to url encoding those changes were moot. I overcomplicated this one, but it's good to go now!

@Abby-Wheelis
Copy link
Member Author

This was merged in PR #1053. The task is done for now, since this feature is not available on staging or production. In order to have this available, we will need to get cyber approval and can refer back to this issue at that time.

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

No branches or pull requests

2 participants