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 and testing #1053

Merged
merged 34 commits into from
Oct 20, 2023

Conversation

Abby-Wheelis
Copy link
Member

See: upload service rewrite for discussion.

While we do not use this anywhere in production now, we would like to restore it at some point and therefore still need tests and a ts version of this file.

Writing the mocks for the file system was difficult, but I think I got something figured out that allows the code to run and to verify that this is working, at least behaving as expected on the phone side.

Abby Wheelis added 18 commits September 28, 2023 09:13
hoping to avoid naming problems when creating the new ts service by the same name
starting to convert the service, but running into a fair number of issues, especially because of complex ionic popups
the upload service called for a log at the error level, adding support for that in logger.ts
could not find file "uploadService.js", needed to carry over the name change to "nguploadService.js"
using async await is a clearer way of handling the asynchronous processes in this file, so it is easier to figure out what is happening and properly set up the typescript version of this service.
we need to take a reason, so why not take it and then call the function? This workflow attempts to take user input in a modal and then pass it to the service's uploadFile function -- note, currently broken!!!
This now appears to be working with the dev server, as the POST requests are going through successfully

The next step is testing and potentially polishing the message popups to the user
there were two errors in VScode, which were also tripping the tests up when I tried to run the uploadFile function -- resolving these did not make the tests run, but it did get past the first of the errors!
tests are not fully running yet, but making progress and need to save before getting the mocks from a remote branch
needed to mock window['resolvelocalfilesystem'] as well as window['cordova'].file
After a lot of work ironing out a mock for the file system layers, a working mock and test has been established

the "reason" passed through remains present throughout the process, confirming in at least a small way that this works
the dev ui server wouldn't run because there were two imports from react-nativ-paper, probably form a bad merge resolution
wrapping up the basic implementation and testing of this upload service, adding comments for clarity
@Abby-Wheelis
Copy link
Member Author

This is now running, and passing the test I created for it. It relies heavily on mocks, specifically for the file reading, fetching, and logging. The test is simple, but proves that parameter data is being passed through the function in the expected manner.

Window.alert is the current mechanism for notifying the user of progress or changes in the upload process, to be reconsidered based on #992.

Abby Wheelis added 4 commits October 5, 2023 11:13
need to pass the file in its original form and not stringify it in order for the data to actually make it to the server

This change is pending restoration of the reason and tz paramenters
In the previous commits, I altered the call to POST, so I needed to alter the way I mocked that call!
@Abby-Wheelis
Copy link
Member Author

This is working now!! Both in the emulator and in the test I wrote. The UI will need some re-vamping (window alerts are not a great way of notifying the user of progress) when we get this back into production, but it is now Reactified and functional.

@Abby-Wheelis
Copy link
Member Author

This PR is dependent on ✏️ Rewrite KVStore, ClientStats, and CommHelper, that one should be merged first.

@Abby-Wheelis Abby-Wheelis marked this pull request as ready for review October 6, 2023 19:32
@JGreenlee
Copy link
Collaborator

Can you change the target of this PR to some other branch, then change it back to service_rewrite_2023?

(#1040 is merged but my commits are still showing up in this PR. I've discovered that toggling the branch forces Github to update the diff)

@Abby-Wheelis Abby-Wheelis changed the base branch from service_rewrite_2023 to master October 13, 2023 14:16
@Abby-Wheelis Abby-Wheelis changed the base branch from master to service_rewrite_2023 October 13, 2023 14:16
@Abby-Wheelis Abby-Wheelis changed the base branch from service_rewrite_2023 to master October 16, 2023 15:36
@Abby-Wheelis Abby-Wheelis changed the base branch from master to service_rewrite_2023 October 16, 2023 15:36
@JGreenlee
Copy link
Collaborator

resolved merge conflicts

Copy link
Collaborator

@JGreenlee JGreenlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this code is not currently being used and we can't try it out end-to-end, but it looks clean and will surely come in handy when it's time.

I have a few questions and requests below.

www/js/control/uploadService.ts Outdated Show resolved Hide resolved
www/__mocks__/fileSystemMocks.ts Show resolved Hide resolved
www/__mocks__/fileSystemMocks.ts Outdated Show resolved Hide resolved
www/js/control/ProfileSettings.jsx Outdated Show resolved Hide resolved
www/js/plugin/logger.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@JGreenlee JGreenlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

www/__tests__/uploadService.test.ts Show resolved Hide resolved
www/json/uploadConfig.json.sample Outdated Show resolved Hide resolved
www/js/control/uploadService.ts Outdated Show resolved Hide resolved
www/js/control/ProfileSettings.jsx Show resolved Hide resolved
www/__tests__/uploadService.test.ts Outdated Show resolved Hide resolved
Abby Wheelis added 5 commits October 19, 2023 09:16
this mock was confusing, so I've added comments to indicate what the two parts do. Perviously, there was no timeout in the if, only in the else, now they are consistent
we want to set the message right away, then have a delay on the response itself
www/__tests__/uploadService.test.ts Outdated Show resolved Hide resolved
@shankari shankari merged commit 412a8ce into e-mission:service_rewrite_2023 Oct 20, 2023
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

Successfully merging this pull request may close these issues.

3 participants