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

emailService Js -> ts #1097

Conversation

niccolopaganini
Copy link
Contributor

  • emailService Js -> ts
  • modified files surrounding emailService

@niccolopaganini niccolopaganini marked this pull request as ready for review November 13, 2023 12:16
@niccolopaganini niccolopaganini changed the base branch from master to service_rewrite_2023 November 13, 2023 12:16
@niccolopaganini
Copy link
Contributor Author

Manual Testing done. Process:

  • ran npm run serve (after activation)
  • connected the phone to Chrome's Developer tools and kept an eye out for issues.
  • Then, I used my address (nseptank@nrel.gov) to send it to my personal email address (write2gamenugget@gmail.com) to make sure (I changed to Shankari's email address )
  • I then went to my personal email address to check the email was received successfully and then the file was present.

@shankari
Copy link
Contributor

@niccolopaganini as we discussed, please add a video of the testing done before requesting review.

@niccolopaganini
Copy link
Contributor Author

@niccolopaganini
Copy link
Contributor Author

Email received (in my work email address)

image

www/index.js Outdated Show resolved Hide resolved
JGreenlee and others added 2 commits November 21, 2023 11:24
- remove unused imports
- use logDebug, logInfo, logWarn, format nicely
- replace ionic.Platform.isIOS / isAndroid with cordova.platformId
@shankari
Copy link
Contributor

shankari commented Nov 22, 2023

@JGreenlee @niccolopaganini I am merging this right now in the interests of progress, but I would like to see tests for this show up fairly soon.

@shankari shankari merged commit 35468e5 into e-mission:service_rewrite_2023 Nov 22, 2023
2 checks passed
@JGreenlee
Copy link
Collaborator

@shankari I didn't ask for unit tests on this PR because I didn't think it would be possible to test the functionality of sending an email (which is basically all this service does)

We could mock the email plugin but then we're just testing the mock. Is there something else here that you see value to testing?

@shankari
Copy link
Contributor

shankari commented Nov 25, 2023

@JGreenlee I agree that mocking is not helpful. However, it seems like it should be possible to use Appium to test that the email client is launched successfully to at least check for issues like https://github.com/katzer/cordova-plugin-email-composer/issues

wrt checking whether the email is actually sent, we just need a service that lets you sent throwaway emails to it and then check their receipt via API. So something like https://testmail.app/ (there might be alternatives, we should look around a bit). Note that we will need to get permission from IT before using any external service, and this is likely to take a few weeks at the minimum.

@JGreenlee
Copy link
Collaborator

it seems like it should be possible to use Appium to test that the email client is launched successfully

Once we have Appium integrated, this might be possible in an Android emulator – but I am pretty sure it's not going to be achievable on the iOS Simulator because it doesn't support email at all.

Even so, I'm not sure if Appium can detect an external app launch. I think Appium attaches itself to the app it's targeted at and I'm not sure if it can detect anything from other apps.

wrt checking whether the email is actually sent

This would be a function of the email client app (e.g. Gmail); why would we have to test an app that isn't ours? Isn't that beyond the scope of this project?

@shankari
Copy link
Contributor

Once we have Appium integrated, this might be possible in an Android emulator – but I am pretty sure it's not going to be achievable on the iOS Simulator because it doesn't support email at all.

Correct, it would only be in an android emulator.

Even so, I'm not sure if Appium can detect an external app launch. I think Appium attaches itself to the app it's targeted at and I'm not sure if it can detect anything from other apps.

It would be interesting to know that. If not, I guess we must maintain a (hopefully short) list of tests that must be performed manually and add this there.

This would be a function of the email client app (e.g. Gmail); why would we have to test an app that isn't ours? Isn't that beyond the scope of this project?

Well, we invoke gmail, and it would be good to know if we do so correctly. For example, if a merge conflict resulted in the email address being munged (e.g. foobar@goobar.commereefe), we would want to know. Similarly, if there is an error in exporting the logs, we would want to know. It would help us find issues like e-mission/e-mission-docs#994

FYI, another email checking service:
https://mailtrap.io/blog/selenium-email-verification/

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