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

fix issue #26 #31

Closed
wants to merge 3 commits into from
Closed

fix issue #26 #31

wants to merge 3 commits into from

Conversation

jackye666
Copy link

Fix #26.
Requirements satisfied:
If you drop a course , then next time you use this project, it will automatically delete the courseID and courseCode from the config file and inform the user.
Design idea:
If the course is dropped, then you cannnot successfully get the relative cousre information from canvas.
For courses downloaded through courseID, I check the return json object. If the json object does not contain "id", then it means that we failed to access the course page. I then delete this courseID from the config.
For courses downloaded through courseCode, I create a list which keeps appending successfully downloaded coursesCode. Then, I compare the list with the courseCode in config file , and delete them.
Quality Assurance:
I conduct peer review with my teamate spikedyf and there is no obviou bug found. What's more, I test on the following test:
I am in course "EECS 477 001 FA 2021" and not in "EECS 496". I enter both course code to the config, and it turns out that "EECS 496" is successfully deleted and the files from "EECS 477 001 FA 2021" are successfully downloaded.

@BoYanZh
Copy link
Owner

BoYanZh commented Dec 8, 2021

Thanks for your contribution and observation. But I think it may be better to leave the config file unchanged but just prompt users that some course Code/course ID is unreachable, then skip these courses in the following steps. If you want to override the config files, it may break the config file under some special conditions.

@jackye666
Copy link
Author

Thank you for your comments, I changed my code so that it only inform the user and won't change the config.

@@ -187,6 +187,7 @@ async def getCourseIdByCourseCodeHelper(self, page, lowerCourseCodes):
if not courses:
return res
for course in courses:
#print(course)
Copy link
Owner

Choose a reason for hiding this comment

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

Commented debug lines should be deleted.

if "id" not in sessRes.keys():
print("There is problem with courseID",courseID)
print("Deleted from config!")
with open(".canvassyncer.json","r+") as f:
Copy link
Owner

Choose a reason for hiding this comment

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

Users may specify the config file through -p.

@@ -11,6 +11,8 @@
import aiofiles
import aiohttp
from tqdm import tqdm
global droppedCourse
droppedCourse={"courseIDs":[],"courseCodes":[]}
Copy link
Owner

Choose a reason for hiding this comment

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

Consider storing the value inside the class.

@@ -208,10 +211,23 @@ async def getCourseIdByCourseCode(self):
endOfPage = True
self.courseCode.update(item)
page += PAGES_PER_TIME
correct_courseCode = []
Copy link
Owner

Choose a reason for hiding this comment

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

Unify the naming style with the project. Use camel case.

correct_courseCode.append(self.courseCode[i])
for i in self.config["courseCodes"]:
if i not in correct_courseCode:
if i not in droppedCourse["courseCodes"]:
Copy link
Owner

Choose a reason for hiding this comment

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

Combine consecutive if with and.

@BoYanZh BoYanZh force-pushed the master branch 3 times, most recently from 84a0c94 to d08a39a Compare August 31, 2022 06:24
@wznmickey wznmickey mentioned this pull request Sep 9, 2022
@BoYanZh BoYanZh closed this Sep 16, 2022
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.

Optimize Drop Course Logic
2 participants