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

Allow the user to import transcripts into the Roadmap Planner #455

Merged
merged 16 commits into from
Mar 10, 2024

Conversation

Awesome-E
Copy link
Member

@Awesome-E Awesome-E commented Mar 4, 2024

Development Notes

Any comments would be very helpful!

  • This would be good to release together with Multiple planner on roadmap page #266 since it currently overrides the user's schedule. (This is still open to discussion though)
  • Lint checks will currently be failing due to updating older files that have ESLint warnings in them.
  • Styles are still using the year modal styles, so this needs to be refactored.
  • Button placement and other layout/styles are not finalized yet.

Description

  • Add package to parse the HTML without injecting content into the page
  • Extract the transferred credits as well as quarters and courses taken at UCI from the HTML Transcript
  • Group these quarters into academic years (in a planner-compatible format)
  • Set the current roadmap's planner based on these year entries

Screenshots

Import Dialog with Information (styles and WIP)
image

Steps to verify/test this change:

  • Verify changes work as expected on staging instance

Final Checks:

  • Verify successful deployment

Issues

Closes #443

Ignoring dependencies array pre-commit hook for now because I want to make sure this doesn't break anything
Currently only works with fall, winter, and spring quarters.
This should make importing transcripts with summer courses work
@Awesome-E Awesome-E added enhancement New feature or request frontend Front End tasks Story Point: 3 backend or full-stack change. maybe adding an endpoint or changing the response of an endpoint + the RFC Request for comment labels Mar 4, 2024
@Awesome-E Awesome-E self-assigned this Mar 4, 2024
@Awesome-E Awesome-E temporarily deployed to staging-455 March 4, 2024 21:02 — with GitHub Actions Inactive
@Awesome-E
Copy link
Member Author

@js0mmer do you think you could take a look at the files failing the lint check and update the dependency arrays for them?

@js0mmer
Copy link
Member

js0mmer commented Mar 5, 2024

Yeah

@js0mmer js0mmer temporarily deployed to staging-455 March 5, 2024 06:41 — with GitHub Actions Inactive
@Awesome-E
Copy link
Member Author

Awesome-E commented Mar 5, 2024

Interesting, yours allowed you to commit even with the dependency array warnings, but mine didn't at first

@Awesome-E Awesome-E temporarily deployed to staging-455 March 5, 2024 06:46 — with GitHub Actions Inactive
@js0mmer
Copy link
Member

js0mmer commented Mar 5, 2024

Yeah, formatting just seemed to be messed up somehow.

@js0mmer
Copy link
Member

js0mmer commented Mar 5, 2024

I will leave the pre-existing dependency warnings to be resolved later, don't want to move too much stuff around in this PR that's out of scope.

@Awesome-E
Copy link
Member Author

I will leave the pre-existing dependency warnings to be resolved later, don't want to move too much stuff around in this PR that's out of scope.

yeah it's not giving an error on commit anymore so not going to worry about it

@Awesome-E Awesome-E temporarily deployed to staging-455 March 5, 2024 06:59 — with GitHub Actions Inactive
@Awesome-E
Copy link
Member Author

Awesome-E commented Mar 6, 2024

Going to request review on this once we decide what the behavior of the import should be. It's currently set to override the entire roadmap because it was easier to use the set hooks rather than checking for (and handling) duplicate cases, but this could be different from the desired behavior.

@Awesome-E Awesome-E temporarily deployed to staging-455 March 6, 2024 09:18 — with GitHub Actions Inactive
@js0mmer
Copy link
Member

js0mmer commented Mar 7, 2024

Now that #440 is merged, I think this would be a great first feature to add to the changelog modal!

this will make it pick up community college courses' units since they don't have a score associated with them
@Awesome-E Awesome-E temporarily deployed to staging-455 March 7, 2024 21:42 — with GitHub Actions Inactive
@Awesome-E
Copy link
Member Author

Included the changes we discussed yesterday regarding quarters and CC course credits @js0mmer
image

@Awesome-E Awesome-E temporarily deployed to staging-455 March 7, 2024 21:53 — with GitHub Actions Inactive
@Awesome-E Awesome-E marked this pull request as ready for review March 7, 2024 21:54
@Awesome-E Awesome-E requested a review from js0mmer March 7, 2024 21:54
@Awesome-E Awesome-E changed the title [WIP] Allow the user to import transcripts into the Roadmap Planner Allow the user to import transcripts into the Roadmap Planner Mar 7, 2024
site/src/pages/RoadmapPage/ImportTranscriptPopup.tsx Outdated Show resolved Hide resolved
site/src/pages/RoadmapPage/ImportTranscriptPopup.tsx Outdated Show resolved Hide resolved
site/src/pages/RoadmapPage/Planner.tsx Outdated Show resolved Hide resolved
@Awesome-E Awesome-E temporarily deployed to staging-455 March 9, 2024 00:16 — with GitHub Actions Inactive
This requires removing all spaces to form IDs
@Awesome-E Awesome-E temporarily deployed to staging-455 March 9, 2024 03:46 — with GitHub Actions Inactive
@Awesome-E Awesome-E temporarily deployed to staging-455 March 9, 2024 03:47 — with GitHub Actions Inactive
@Awesome-E Awesome-E requested a review from js0mmer March 9, 2024 03:47
@Awesome-E
Copy link
Member Author

Still not a huge fan of just removing spaces but I do agree that it's better than essentially reusing 40 lines of code.

Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

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

lgtm

@Awesome-E Awesome-E merged commit 08517ad into master Mar 10, 2024
3 checks passed
@Awesome-E Awesome-E deleted the import-transcript branch March 10, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend Front End tasks RFC Request for comment Story Point: 3 backend or full-stack change. maybe adding an endpoint or changing the response of an endpoint + the
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import unofficial transcript
2 participants