-
Notifications
You must be signed in to change notification settings - Fork 15
Fixing the List of Summer Terms on the Request Summer Proposal page #1767
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
base: development
Are you sure you want to change the base?
Changes from all commits
f341849
0615297
36ef8eb
48477ca
bd79a13
4114367
c8274a2
f00a23f
005c3d1
fd09690
928f1e3
818663c
ebb2bad
707b93a
810640c
9bd0254
dbabdb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
| from app.models.term import Term | ||
| from app.models.attachmentUpload import AttachmentUpload | ||
| from app.logic.fileHandler import FileHandler | ||
| from app.logic.utils import selectSurroundingTerms, getFilesFromRequest | ||
| from app.logic.utils import selectSurroundingTerms, getFilesFromRequest, selectAllSummerTerms | ||
| from app.logic.minor import ( | ||
| changeProposalStatus, | ||
| createOtherEngagement, | ||
|
|
@@ -120,13 +120,16 @@ def createSummerExperienceRequest(username): | |
| createSummerExperience(username, request.form) | ||
| flash("Proposal successfully created.", "success") | ||
| return redirect(url_for('minor.viewCceMinor', username=username, tab="manageProposals")) | ||
|
|
||
| student = User.get_by_id(username) | ||
| year_name = User.rawClassLevel | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be classLevel instead of year_name |
||
|
|
||
| summerTerms = selectSurroundingTerms(g.current_term, summerOnly=True) | ||
| summerTerms = selectAllSummerTerms(g.current_term, student) | ||
|
|
||
| return render_template("minor/summerExperience.html", | ||
| selectableTerms = summerTerms, | ||
| contentAreas = [], | ||
| user = User.get_by_id(username), | ||
| user = student, | ||
| ) | ||
|
|
||
| @minor_bp.route('/cceMinor/<username>/getEngagementInformation/<type>/<term>/<id>', methods=['GET']) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,43 @@ def selectSurroundingTerms(currentTerm, prevTerms=2, summerOnly=False): | |
|
|
||
| return surroundingTerms | ||
|
|
||
| def selectAllSummerTerms(currentTerm, student): | ||
| """ | ||
| Select the summer terms during which a CCE Minor student could be enrolled. | ||
|
|
||
| The user record does not store an admission date, so the admission academic | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docstrings should give a reason for input and what returns and a single sentence of what it should do. |
||
| year is inferred from the student's current class level. The range never | ||
| extends beyond the student's inferred final school year; students marked as | ||
| ``Graduating`` are treated as fifth-year/fall-graduating students. | ||
| """ | ||
| classYears = { | ||
| "Freshman": 1, | ||
| "Sophomore": 2, | ||
| "Junior": 3, | ||
| "Senior": 4, | ||
| "Graduating": 5, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. graduating is troublesome classLevel because when you run this command: SELECT * FROM user AS u WHERE u.rawClassLevel = 'Graduating' AND u.isGraduated = TRUE; you will see those two attributes can occur at the same time. |
||
| } | ||
|
|
||
| if student.hasGraduated or student.rawClassLevel not in classYears: | ||
| return [] | ||
|
|
||
| classYear = classYears[student.rawClassLevel] | ||
| # A Spring/Summer term belongs to the academic year that began the prior fall. | ||
| academicYearStart = (currentTerm.year if currentTerm.description.startswith("Fall") | ||
| else currentTerm.year - 1) | ||
| inferredAdmissionYear = academicYearStart - (classYear - 1) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what we can do here is a way to shrink these lines 63,65,66 to two lines in two of these forms: eg 1: eg2: |
||
|
|
||
| firstSummer = inferredAdmissionYear + 1 | ||
| lastSummer = inferredAdmissionYear + max(4, classYear) - 1 | ||
| if currentTerm.description.startswith("Summer"): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. currentTerm.isSummer should and can be use here as we have a field for this. |
||
| lastSummer = max(lastSummer, currentTerm.year) | ||
|
|
||
| return (Term.select() | ||
| .where(Term.isSummer, | ||
| Term.year >= firstSummer, | ||
| Term.year <= lastSummer) | ||
| .order_by(Term.termOrder)) | ||
|
|
||
| def getStartofCurrentAcademicYear(currentTerm): | ||
| if ("Summer" in currentTerm.description) or ("Spring" in currentTerm.description): | ||
| fallTerm = Term.select().where(Term.year==currentTerm.year-1, Term.description == f"Fall {currentTerm.year-1}").get() | ||
|
|
@@ -97,4 +134,3 @@ def setRedirectTarget(target): | |
| return: None | ||
| """ | ||
| session["redirectTarget"] = target | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,8 +5,6 @@ | |
| {{ super() }} | ||
| <script type="module" src="/static/js/minorProfilePage.js"></script> | ||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery.inputmask/3.3.4/jquery.inputmask.bundle.min.js"></script> | ||
| <script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-select/1.5.4/bootstrap-select.js"></script> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are these remove? |
||
| <script type="module" src="{{ url_for('static', filename='js/profile.js') }}"></script> | ||
|
|
||
| {% endblock %} | ||
|
|
||
|
|
@@ -47,7 +45,7 @@ <h4>Proposal for Community-Engaged Summer Experience</h4> | |
| </option> | ||
| {% endif %} | ||
| {% for term in selectableTerms %} | ||
| <option value="{{ term }}" | ||
| <option value="{{ term.id }}" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| {% if proposal and proposal.term == term %} selected {% endif %}> | ||
| {{ term.description }} | ||
| </option> | ||
|
|
@@ -177,4 +175,4 @@ <h4>Experience Information</h4> | |
| </div> | ||
| </form> | ||
| </div> | ||
| {% endblock %} | ||
| {% endblock %} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| import pytest | ||
| from types import SimpleNamespace | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do not use SimpleNamespace unless we are trying to let's say fake a request in the test so that we can avoid create a whole class for it. For instance: mockRequestProposalObject = SimpleNamespace( |
||
|
|
||
| from app.models import mainDB | ||
| from app.models.term import Term | ||
| from app.logic.utils import selectAllSummerTerms | ||
|
|
||
|
|
||
|
|
||
|
|
@@ -78,3 +81,48 @@ def test_isFutureTerm(): | |
| # current term | ||
| assert testCurrentTerm.isFutureTerm == False | ||
| transaction.rollback() | ||
|
|
||
|
|
||
| @pytest.mark.integration | ||
| @pytest.mark.parametrize( | ||
| "class_level, current_description, current_year, expected_years", | ||
| [ | ||
| ("Freshman", "Fall 2090", 2090, [2091, 2092, 2093]), | ||
| ("Sophomore", "Summer 2091", 2091, [2090, 2091, 2092]), | ||
| ("Senior", "Fall 2090", 2090, [2088, 2089, 2090]), | ||
| ], | ||
| ) | ||
| def test_selectAllSummerTerms_uses_estimated_enrollment_window( | ||
| class_level, current_description, current_year, expected_years): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sure to test graduating too as you have special logic for graduating when populating for the student. |
||
| with mainDB.atomic() as transaction: | ||
| for year in range(2087, 2096): | ||
| Term.create(description=f"Summer {year}", | ||
| year=year, | ||
| academicYear=f"{year - 1}-{year}", | ||
| isSummer=True, | ||
| isCurrentTerm=False, | ||
| termOrder=f"{year}-2") | ||
|
|
||
| student = SimpleNamespace(rawClassLevel=class_level, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the above comment regarding simplenamespace convert those so that it reflects the dictionary usage. |
||
| hasGraduated=False) | ||
| currentTerm = SimpleNamespace(description=current_description, | ||
| year=current_year) | ||
|
|
||
| assert expected_years == [ | ||
| term.year for term in selectAllSummerTerms(currentTerm, student) | ||
| ] | ||
| transaction.rollback() | ||
|
|
||
|
|
||
| def test_selectAllSummerTerms_excludes_graduated_students(): | ||
| student = SimpleNamespace(rawClassLevel="Senior", hasGraduated=True) | ||
| currentTerm = SimpleNamespace(description="Summer 2090", year=2090) | ||
|
|
||
| assert selectAllSummerTerms(currentTerm, student) == [] | ||
|
|
||
|
|
||
| def test_selectAllSummerTerms_excludes_unknown_class_levels(): | ||
| student = SimpleNamespace(rawClassLevel=None, hasGraduated=False) | ||
| currentTerm = SimpleNamespace(description="Summer 2090", year=2090) | ||
|
|
||
| assert selectAllSummerTerms(currentTerm, student) == [] | ||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incase username is not student I know this is not the case above it should be as cceMinor is reserve for students only so we need to ensure isStudent is user here