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

feat: UTRP v2 migration #292

Merged
merged 18 commits into from
Oct 15, 2024
Merged

feat: UTRP v2 migration #292

merged 18 commits into from
Oct 15, 2024

Conversation

doprz
Copy link
Collaborator

@doprz doprz commented Oct 12, 2024

TODO

  • activeSchedule is not updated after switching to the new schedule
  • If a schedule with the name UTRP v1 Migration already exists for some reason, it might cause bugs
  • After redirecting to UT login (if not logged in), retry course migration (TODO)
  • Add migration to onUpdate
  • Calling UTRP v2 migration from onUpdate didn't seem to work properly with pnpm run build:watch but manually pressing the Migrate UTRP v1 courses button worked. pnpm run dev works too.

Issues that need to be resolved

Quoted from @Samathingamajig

  • Doesn't work when not logged into ut
  • Chrome warns in the console that in the future, cookies will not work when we do a network request like how we are doing it now, so might need to open a new tab (ideally a hidden/background tab, if those exist) instead
  • Doesn't yet try to read from chrome.storage.sync for the existing classes

It does work enough such that a url to the class info page can get all the course info needed

Huly®: UTRP-289


This change is Reviewable

src/shared/util/checkLoginStatus.ts Show resolved Hide resolved
src/pages/background/lib/migrateUTRPv1Courses.ts Outdated Show resolved Hide resolved
src/views/hooks/useSchedules.ts Show resolved Hide resolved
src/views/lib/courseMigration.ts Outdated Show resolved Hide resolved
@doprz doprz requested review from sghsri and Razboy20 October 13, 2024 16:06
@doprz doprz marked this pull request as ready for review October 13, 2024 16:06
Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

Modify text to remove migration language and match the language on the Figma

Change button color to ut-green to make the button stand out more

Move the announcement section above the Useful Links section

@doprz doprz requested a review from DereC4 October 13, 2024 16:18
@doprz
Copy link
Collaborator Author

doprz commented Oct 13, 2024

Modify text to remove migration language and match the language on the Figma

Change button color to ut-green to make the button stand out more

Move the announcement section above the Useful Links section

Some thoughts:

  • I'm ok with using ut-burntorange or ut-green but I do like ut-burntorange more.
  • Since the migration is a one time thing it shouldn't be above the Useful Links section because of UI/UX principles unless we add a toggle to only show it once.

@IsaDavRod
Copy link
Member

Modify text to remove migration language and match the language on the Figma
Change button color to ut-green to make the button stand out more
Move the announcement section above the Useful Links section

Some thoughts:

  • I'm ok with using ut-burntorange or ut-green but I do like ut-burntorange more.
  • Since the migration is a one time thing it shouldn't be above the Useful Links section because of UI/UX principles unless we add a toggle to only show it once.

Thank you for your thoughts. Let’s use ut-green to make it stand out. Since we’re adding courses, we can add a green button in the branding for that purpose.

For now, the migration announcement is only displayed above the Useful Links section. Most users will open the calendar for the first time and experience immediate stress when they see that their saved schedule and hard work is gone. If the migration is automatic and the button serves as a “backup” in case of failure, then we can move the Migration Announcement section below the Useful Links section.

If we want to stick to UI/UX principles, the Migration section should only appear once and be hidden if the schedule is successfully migrated. In that case, we can implement a toggle “migrationComplete” that stores and sets to true after the user completes their migration.

Style changes:
image

@DereC4
Copy link
Member

DereC4 commented Oct 13, 2024

Derek Chen will return

src/pages/background/lib/migrateUTRPv1Courses.ts Outdated Show resolved Hide resolved
src/pages/background/lib/migrateUTRPv1Courses.ts Outdated Show resolved Hide resolved
src/views/components/calendar/Calendar.tsx Show resolved Hide resolved
src/views/components/settings/Settings.tsx Outdated Show resolved Hide resolved
src/views/components/settings/Settings.tsx Outdated Show resolved Hide resolved
@doprz doprz added this to the v2.0.0 milestone Oct 14, 2024
src/views/components/settings/Settings.tsx Outdated Show resolved Hide resolved
src/views/components/settings/Settings.tsx Outdated Show resolved Hide resolved
src/pages/background/lib/migrateUTRPv1Courses.ts Outdated Show resolved Hide resolved
src/views/contexts/SentryContext.tsx Outdated Show resolved Hide resolved
src/pages/background/lib/createSchedule.ts Show resolved Hide resolved
src/views/components/common/MigrationDialog.tsx Outdated Show resolved Hide resolved
src/views/contexts/SentryContext.tsx Show resolved Hide resolved
src/pages/background/lib/migrateUTRPv1Courses.ts Outdated Show resolved Hide resolved
@Razboy20 Razboy20 force-pushed the feature/v2-migration branch from a86328b to 16f6e56 Compare October 14, 2024 18:51
@doprz doprz added ready-for-review dependencies Pull requests that update a dependency file bugfix UI/UX-figma PRs that fulfill a task on the UI/UX & Feature Roadmap and removed bugfix labels Oct 14, 2024
Copy link
Collaborator Author

@doprz doprz left a comment

Choose a reason for hiding this comment

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

LGTM

@Samathingamajig
Copy link
Collaborator

Samathingamajig commented Oct 14, 2024

note: we're going to (attempt to) do a large-scale test at the meeting tonight

@Razboy20 Razboy20 force-pushed the feature/v2-migration branch from 63d5f64 to c58bb2d Compare October 14, 2024 21:16
Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

Tested it, it works. LGTM

@Razboy20 Razboy20 merged commit d22237d into main Oct 15, 2024
27 checks passed
@doprz doprz deleted the feature/v2-migration branch October 15, 2024 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file feature ready-for-review UI/UX-figma PRs that fulfill a task on the UI/UX & Feature Roadmap utrp-v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants