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

Added function to calculate cumulative GPA(including unfinished semesters) #140

Merged
merged 6 commits into from
Mar 18, 2020

Conversation

Suhas-13
Copy link
Member

@Suhas-13 Suhas-13 commented Mar 17, 2020

Closes #1

Calculate cumulative GPA and added optional parameter creditHour to Course class.

Signed-off-by: Suhas Hariharan hariharan774531@sas.edu.sg

@Suhas-13 Suhas-13 changed the title Calculate cumulative gpa, adds optional parameter creditHour to Cours… Calculate cumulative gpa up to current semester, and adds optional parameter creditHour to Courses Mar 17, 2020
@Suhas-13 Suhas-13 requested a review from gary-kim March 17, 2020 01:28
@Suhas-13
Copy link
Member Author

Sorry I messed up the other branch so I just created a new pull request/branch with it all in one commit.

@Suhas-13 Suhas-13 changed the title Calculate cumulative gpa up to current semester, and adds optional parameter creditHour to Courses added function to calculate cumulative GPA Mar 17, 2020
@Suhas-13 Suhas-13 changed the title added function to calculate cumulative GPA Added function to calculate cumulative GPA(only completed semesters) Mar 17, 2020
@gary-kim gary-kim added the feature New feature addition label Mar 17, 2020
@gary-kim gary-kim added this to the Open Beta v0.21.0 milestone Mar 17, 2020
@Suhas-13
Copy link
Member Author

Suhas-13 commented Mar 17, 2020

I just added something so it should calculate cumulative including current semester, I used the begdate thing to figure out whether the semester was over or not. I'm calculating credit hours for the non grade history grades based on the amount of blocks they take up, with an exception for interim which would be 0.25. Are there any other prefixes for interim other than IS on powerschool?

@Suhas-13 Suhas-13 changed the title Added function to calculate cumulative GPA(only completed semesters) Added function to calculate cumulative GPA(including unfinished semesters) Mar 17, 2020
Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

We already have a function for that. Take the one here and export it and use that instead.
https://github.com/gary-kim/saspes/blob/7722c9ea733cb67f0056f85a66a78e4abff2a285/src/js/helpers.js#L116

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

Just from a cursory look at the code. I haven't tested this yet.

src/js/saspowerschoolff.js Outdated Show resolved Hide resolved
src/js/saspowerschoolff.js Outdated Show resolved Hide resolved
src/js/saspowerschoolff.js Outdated Show resolved Hide resolved
@Suhas-13
Copy link
Member Author

Is it okay with you if I take total_add out of the calculate_gpa function and rename it calcualte_credit_hours?

@gary-kim
Copy link
Member

Is it okay with you if I take total_add out of the calculate_gpa function and rename it calcualte_credit_hours?

Yes. You don't have to ask :).

@Suhas-13
Copy link
Member Author

I made the changes, does this look ready to merge?

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

Just tried testing and noticed a problem but other then that, I think we should be good.

src/js/saspowerschoolff.js Outdated Show resolved Hide resolved
src/js/saspowerschoolff.js Outdated Show resolved Hide resolved
@Suhas-13
Copy link
Member Author

Suhas-13 commented Mar 17, 2020

Not quite sure what happened, I just commited your suggestions than ran git rebase HEAD~7 --signoff to add signoffs as per what github prompted me to do to sign it, now it seems that theres conflicts?

@Suhas-13
Copy link
Member Author

Could I just do git reset --hard 25edb97 to go back to before?

@gary-kim
Copy link
Member

You shouldn't need to. You can just run git fetch && git rebase origin/master and that will fix the conflicts.

@Suhas-13
Copy link
Member Author

Okay, it seems to have added a bunch more files to the files changed as well for this commit, would that all be resolved by rebasing?

Suhas Hariharan added 5 commits March 17, 2020 22:45
…e class

Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
…ious calculate_credit_hours function

Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
…ive gpa message

Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
@Suhas-13
Copy link
Member Author

Should be ready to merge?

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

A lot of this is probably going to be moved into a separate file when we convert to using Vue but for now, this looks great!

let include_current_semester = false;
if (current_courses.length !== 0) {
for (let i = 0; i < current_courses.length; i++) {
if (current_courses[i].link.includes("begdate")) {
Copy link
Member

@gary-kim gary-kim Mar 17, 2020

Choose a reason for hiding this comment

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

The better way to do this would be something like

Suggested change
if (current_courses[i].link.includes("begdate")) {
if (new URL(current_courses[i].link).searchParams.get("begdate")) {

but we can deal with that during the cleanup if you'd like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, thanks okay if I merge it now?

Copy link
Member

Choose a reason for hiding this comment

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

If the PR's approved by someone with write access, you don't need to ask :)

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

One more thing I noticed that's probably better taken care of in cleanup.

const $prev_course = element_list[t];
// Creates course object with each course from grade history page
const course = new Course($prev_course.getElementsByTagName("td")[0].textContent.trim(), "",
$prev_course.getElementsByTagName("td")[1].textContent.trim(), 0, "");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$prev_course.getElementsByTagName("td")[1].textContent.trim(), 0, "");
$prev_course.getElementsByTagName("td")[1].textContent.trim());

@Suhas-13 Suhas-13 merged commit c1e0a1a into master Mar 18, 2020
@gary-kim gary-kim deleted the calculate-cumulative-gpa branch March 18, 2020 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show the effect of current semester to cumulative GPA
2 participants