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

Investigate crash on updatePrefReminderTime #1052

Open
sebastianbarry opened this issue Feb 20, 2024 · 3 comments
Open

Investigate crash on updatePrefReminderTime #1052

sebastianbarry opened this issue Feb 20, 2024 · 3 comments

Comments

@sebastianbarry
Copy link

sebastianbarry commented Feb 20, 2024

@JGreenlee found a critical error in e-mission/e-mission-phone#1130, where anytime a user would update their reminder time preference for their profile from the Profile tab, the app would crash.

Investigation:

  • Why did this slip through testing?
  • Expand/fix tests
@sebastianbarry
Copy link
Author

In my original PR for Rewrite notifScheduler, I changed the updatePrefReminderTime function here: https://github.com/e-mission/e-mission-phone/pull/1092/files#diff-2aa4eaf8de8847ccdbee42704ea12b0b1dbda29ac50285ae7718b49d2610402a

  async function updatePrefReminderTime(storeNewVal = true, newTime) {
  async function updatePrefReminderTime(storeNewVal = true, newTime) {
    console.log(newTime);
    console.log(newTime);
    if (storeNewVal) {
    if (storeNewVal) {
-      const m = moment(newTime);
+      const m = DateTime.fromISO(newTime);
      // store in HH:mm
      // store in HH:mm
      NotificationScheduler.setReminderPrefs({ reminder_time_of_day: m.format('HH:mm') }).then(
      setReminderPrefs(
        () => {
        { reminder_time_of_day: m.toFormat('HH:mm') },
          refreshNotificationSettings();
        uiConfig.reminderSchemes,
        },
        isScheduling,
      );
        setIsScheduling,
        scheduledPromise,
      ).then(() => {
        refreshNotificationSettings();
      });
    }
    }
  }

I used the function DateTime.fromISO() which I was originally using at the beginning of the PR, but this function did not server the purpose I needed and so I switched the instances of fromISO() to fromJSDate(), but I missed this one because it is in ProfileSettings.jsx and not in notifScheduler.ts. This was an oversight of mine.

This missed automated unit testing because it is in a separate file, since I wasn't rewriting all of ProfileSettings.jsx, I did not consider writing automated unit tests for this.

As for why I missed functionality testing, I don't recall seeing the "change pref reminder time" button that you were using @JGreenlee
image
The config file I was using for testing is here: https://raw.githubusercontent.com/sebastianbarry/nrel-openpath-deploy-configs/add-doee-electricbike-proj-config/configs/doee-electricbike-proj.nrel-op.json Could I have been missing a configuration value in here that caused me to not have the button appear?

@JGreenlee
Copy link

@sebastianbarry notifScheduler only schedules notifications for configurations that have reminderSchemes.
If reminderSchemes is not present, we use push notifications for reminders and notifScheduler never even gets called.

"denver-casr" has reminderSchemes. So does "dev-emulator-timeuse". I would test using one of those

@JGreenlee
Copy link

This missed automated unit testing because it is in a separate file, since I wasn't rewriting all of ProfileSettings.jsx, I did not consider writing automated unit tests for this.

This makes sense. ProfileSettings is quite big and although we have added tests for the services it invokes, we don't have tests for ProfileSettings itself. I think that would support the argument for integration testing in the future.

For now, maybe you could add more tests for the notifScheduler functions with invalid inputs. We can make sure the functions can't do anything catastrophically bad, even if their inputs are bad?

Like for example, a test to verify that the popup I added (in notifScheduler in e84e816) works when there is an invalid datetime

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