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.

Sign up for GitHub

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

Move disclosures to fetch-logic #8008

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Move disclosures to fetch-logic #8008

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
715199f
Move disclosures to fetch-logic
wpears Nov 6, 2023
79c3f37
set school_primary_alias once
wpears Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/functional-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,13 @@ jobs:
run: ./refresh-data.sh test.sql.gz

- name: Run Cypress
uses: cypress-io/github-action@v5
uses: cypress-io/github-action@v6.6.0
with:
build: yarn build
start: python cfgov/manage.py runserver 0.0.0.0:8000
wait-on: 'http://localhost:8000'
spec: test/cypress/integration/paying-for-college/disclosures/disclosures-basics.cy.js

env:
MAPBOX_ACCESS_TOKEN: ${{ secrets.MAPBOX_ACCESS_TOKEN }}
CYPRESS_ENVIRONMENT: github
Expand Down
14 changes: 7 additions & 7 deletions cfgov/paying_for_college/jinja2/paying-for-college/disclosure.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ <h2 class="verify_prompt">
</h2>
<form class="verify_form">
<div class="verify_school">
{{school.primary_alias}}
{% if school%}{{school.primary_alias}}{% endif %}
Copy link
Member

@anselmbradford anselmbradford Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of all the conditionals, would it make sense to have something like this at the top:

{% set school_primary_alias = school.primary_alias if school else '' %}

And then {{ school_primary_alias }} in the spots where it is used?

If you do keep the conditions, perhaps format them the same as {% if school %}, instead of a mix of that and {% if school%}

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, I just did it with a mass replace in vim ^^;

Copy link
Member Author

@wpears wpears Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Done in 79c3f37

</div>
<div class="verify_location">
{% if school %}{{school.city}}, {{school.state}}{% endif %}
Expand Down Expand Up @@ -258,7 +258,7 @@ <h2 class="step_heading">
<p class="step_intro">
Here is your financial aid offer from
<span
id="intro__school-name">{{school.primary_alias}}</span>. Please review the amounts provided by
id="intro__school-name">{% if school%}{{school.primary_alias}}{% endif %}</span>. Please review the amounts provided by
your school below and make any necessary changes
or add any missing information. Please note any
changes you make here will not change your
Expand Down Expand Up @@ -886,7 +886,7 @@ <h4 class="aid-form_summary-heading">
<p>
This is an estimate of the remaining costs
of studying your program at
{{school.primary_alias}} for one year.
{% if school %}{{school.primary_alias}}{% endif %} for one year.
</p>
</div>
</div>
Expand Down Expand Up @@ -1742,7 +1742,7 @@ <h4 class="aid-form_summary-heading">
<p>
This is an estimate of the remaining costs
of studying your program at
{{school.primary_alias}} for one year based
{% if school %}{{school.primary_alias}}{% endif %} for one year based
on your inputs above.
</p>
</div>
Expand Down Expand Up @@ -1873,7 +1873,7 @@ <h4 class="aid-form_summary-heading">
After all the grants, scholarships, loans,
and personal contributions, this is how
much you still need to pay to attend
{{school.primary_alias}} for one year.
{% if school %}{{school.primary_alias}}{% endif %} for one year.
</p>
</div>
<div class="aid-form_summary debt-summary
Expand Down Expand Up @@ -2003,7 +2003,7 @@ <h3 class="criteria_heading">
<p class="content_grad-cohort">
For first-time students enrolled full-time
in {{program.program_name}} at
{{school.primary_alias}},
{% if school %}{{school.primary_alias}}{% endif %},
{{program.completers}} out of
{{program.completion_cohort}}
graduated.
Expand Down Expand Up @@ -2942,7 +2942,7 @@ <h2 class="step_heading step_settlement">
It's estimated you'll owe <span
data-financial="loanLifetime">[XX]</span>
to complete this program in {{program.program_name}} at
{{school.primary_alias}}. Do you better
{% if school %}{{school.primary_alias}}{% endif %}. Do you better
understand how this may impact your future
finances?
</h2>
Expand Down
79 changes: 18 additions & 61 deletions cfgov/unprocessed/apps/paying-for-college/js/disclosures/dispatchers/get-api-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,20 @@
import { promiseRequest } from '../utils/promise-request.js';
import financialView from '../views/financial-view.js';

/**
* getApi - Make an API request to the endpoint specified with parameters specified
* @param {string} url - URL of API endpoint
* @returns {object} Promise
*/
function getApi(url) {
return new Promise(function (resolve, reject) {
promiseRequest('GET', url)
.then(function (resp) {
resolve(resp);
})
.catch(function (error) {
reject(new Error(error));
});
});
}

const getApiValues = {
values: {},
constants: function () {
const urlBase = document.querySelector('main').getAttribute('data-context');
const url =
'/' + urlBase + '/understanding-your-financial-aid-offer/api/constants/';
return getApi(url);
return fetch(url);
},

expenses: function () {
const urlBase = document.querySelector('main').getAttribute('data-context');
const url =
'/' + urlBase + '/understanding-your-financial-aid-offer/api/expenses/';

return getApi(url);
return fetch(url);
},

fetchSchoolData: function (iped) {
Expand All @@ -44,15 +26,9 @@ const getApiValues = {
iped +
'/';

return new Promise(function (resolve ) {
promiseRequest('GET', url)
.then(function (resp) {
resolve(resp);
})
.catch(function (error) {
financialView.missingData('school');
new Error(error);
});
return fetch(url).catch(function (error) {
financialView.missingData('noSchool');
return new Error(error);
});
},

Expand All @@ -77,26 +53,9 @@ const getApiValues = {
pid +
'/';

// $.ajax({
// url: url,
// dataType: 'json',
// success: function (resp) {
// return resp;
// },
// error: function (/* req, status, err */) {
// financialView.missingData('program');
// },
// });

return new Promise(function (resolve ) {
promiseRequest('GET', url)
.then(function (resp) {
resolve(resp);
})
.catch(function () {
financialView.missingData('school');
// reject(new Error(error));
});
return fetch(url).catch(function (error) {
financialView.missingData('noProgram');
return new Error(error);
});
},

Expand All @@ -114,14 +73,14 @@ const getApiValues = {
url += '_' + pid + '/';
}

return getApi(url);
return fetch(url);
},

schoolData: function (iped, pid) {
return Promise.all([
this.fetchSchoolData(iped),
this.fetchProgramData(iped, pid),
this.fetchNationalData(iped, pid),
this.fetchSchoolData(iped).then((v) => v.json()),
this.fetchProgramData(iped, pid).then((v) => v.json()),
this.fetchNationalData(iped, pid).then((v) => v.json()),
]);
},

Expand All @@ -130,16 +89,14 @@ const getApiValues = {
const warning = document
.querySelector('[data-warning]')
.getAttribute('data-warning');
if (warning !== '' && typeof warning !== 'undefined') {
if (warning && !window.Cypress) {
financialView.missingData(warning);
Copy link
Member

Choose a reason for hiding this comment

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

Should this code know about Cypress? It seems like whatever the need is here should be offloaded to the test logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is kinda gross, the problem is though that the data isn't available in the test db, so this warning code will always trigger, even though the actual http requests are stubbed.

Putting this logic here allows Cypress to skip the warning code (which we want) and, in normal use of the app, allows the warning conditional to trigger when we need it.

I think maybe the best solution would be rearchitecting how the warning logic works/how the warning is propagated from the backend to the frontend, but that seemed out of scope.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'd be happy to fast-follow that because this is pretty gross. 😁

const deferred = {};
deferred.promise = new Promise((resolve, reject) => {
deferred.resolve = resolve;
deferred.reject = reject;
});
return deferred;
return Promise.resolve();
}
return Promise.all([this.constants(), this.expenses()]);
return Promise.all([
this.constants().then((v) => v.json()),
this.expenses().then((v) => v.json()),
]);
},
};

Expand Down
15 changes: 6 additions & 9 deletions cfgov/unprocessed/apps/paying-for-college/js/disclosures/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import $ from './utils/dollar-sign.js';
import fetch from './dispatchers/get-api-values.js';
import getApiValues from './dispatchers/get-api-values.js';
import verifyOffer from './dispatchers/post-verify.js';
import financialModel from './models/financial-model.js';
import schoolModel from './models/school-model.js';
Expand Down Expand Up @@ -38,10 +38,9 @@ const ready = function (callback) {
const app = {
urlValues: {},
init: function () {
// jquery promise to delay full model creation until ajax resolves
fetch.initialData().then((resp) => {
const constants = JSON.parse(resp[0].responseText);
const expenses = JSON.parse(resp[1].responseText);
getApiValues.initialData().then((resp) => {
if (!resp) return;
const [constants, expenses] = resp;
financialModel.init(constants[0]);
financialView.init();
if (location.href.indexOf('about-this-tool') === -1) {
Expand All @@ -51,12 +50,10 @@ const app = {
if (getUrlOfferExists()) {
// Check for URL offer data
this.urlValues = getUrlValues();
fetch
getApiValues
.schoolData(this.urlValues.collegeID, this.urlValues.programID)
.then((respArr) => {
const schoolData = JSON.parse(respArr[0].responseText);
const programData = JSON.parse(respArr[1].responseText);
const nationalData = JSON.parse(respArr[1].responseText);
const [schoolData, programData, nationalData] = respArr;
const data = {};
Object.assign(data, schoolData, programData, nationalData);
const schoolValues = schoolModel.init(
Expand Down
28 changes: 7 additions & 21 deletions cfgov/unprocessed/apps/paying-for-college/js/disclosures/utils/dollar-sign.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**

Check warning on line 1 in cfgov/unprocessed/apps/paying-for-college/js/disclosures/utils/dollar-sign.js

View workflow job for this annotation

GitHub Actions / frontend 

Missing JSDoc @returns declaration
*
* @param selector

Check warning on line 3 in cfgov/unprocessed/apps/paying-for-college/js/disclosures/utils/dollar-sign.js

View workflow job for this annotation

GitHub Actions / frontend 

Missing JSDoc @param "selector" description

Check warning on line 3 in cfgov/unprocessed/apps/paying-for-college/js/disclosures/utils/dollar-sign.js

View workflow job for this annotation

GitHub Actions / frontend 

Missing JSDoc @param "selector" type
*/
function Query(selector) {
this.elements = [];
Expand All @@ -17,17 +17,11 @@
if (typeof selector === 'string' && selector !== '') {
this.elements = document.querySelectorAll(selector);
}

// if ( this.elements.length > 1 ) {
// return this.elements;
// } else {
// return this.elements[0];
// }
}

Query.prototype.attr = function (name, value) {
if (typeof value === 'undefined') {
return this.elements[0].getAttribute(name);
return this.elements.length ? this.elements[0].getAttribute(name) : null;
} else {
this.elements.forEach((elem) => {
elem.setAttribute(name, value);
Expand All @@ -36,13 +30,13 @@
};

Query.prototype.cloner = function () {
return this.elements[0].cloneNode(true);
return this.elements.length ? this.elements[0].cloneNode(true) : null;
};

Query.prototype.text = function (value) {
// getter
if (typeof value === 'undefined') {
return this.elements[0].textContent;
return this.elements.length ? this.elements[0].textContent : null;
}
//setter
else {
Expand All @@ -55,7 +49,7 @@
Query.prototype.val = function (value) {
// getter
if (typeof value === 'undefined' && this.elements.length > 0) {
return this.elements[0].value;
return this.elements.length ? this.elements[0].value : null;
}
//setter
else {
Expand Down Expand Up @@ -146,19 +140,11 @@
};

Query.prototype.height = function () {
if (this.elements.length > 0) {
return this.elements[0].offsetHeight;
} else {
return null;
}
return this.elements.length ? this.elements[0].offsetHeight : null;
};

Query.prototype.top = function () {
if (this.elements.length > 0) {
return this.elements[0].offsetTop;
} else {
return null;
}
return this.elements.length ? this.elements[0].offsetTop : null;
};

Query.prototype.listen = function (eventType, callback) {
Expand All @@ -168,7 +154,7 @@
};

Query.prototype.tagName = function () {
return this.elements[0].tagName;
return this.elements.length ? this.elements[0].tagName : null;
};

Query.prototype.addClass = function (classNames) {
Expand Down
30 changes: 0 additions & 30 deletions cfgov/unprocessed/apps/paying-for-college/js/disclosures/utils/promise-request.js
View file
Open in desktop

This file was deleted.

15 changes: 7 additions & 8 deletions cfgov/unprocessed/apps/paying-for-college/js/disclosures/views/financial-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,11 @@ const financialView = {
*/
updateViewWithProgram: function (values, urlValues) {
// Update program length
const lengthExists = Object.prototype.hasOwnProperty.call(urlValues, 'urlProgramLength');
if ( lengthExists ) {
const lengthExists = Object.prototype.hasOwnProperty.call(
urlValues,
'urlProgramLength',
);
if (lengthExists) {
this.$programLength.val(urlValues.urlProgramLength / 12).change();
} else {
this.$programLength.val(values.programLength).change();
Expand Down Expand Up @@ -959,18 +962,14 @@ const financialView = {
*/
missingData: function (dataType) {
$('.verify_wrapper').hide();
if (
$('[data-missing-data-error]:not([style*="display: none"]').length === 0
) {
$('[data-missing-data-error="' + dataType + '"]').show();
}
$('[data-missing-data-error="' + dataType + '"]').show();
},

/**
* Listener function for change events on financial INPUT fields
*/
financialInputChangeListener: function () {
$('[nancial]').each((elmo) => {
$('[data-financial]').each((elmo) => {
elmo.addEventListener('change', (event) => {
const dataFinancial = event.target.dataset.financial;
if (dataFinancial) {
Expand Down
Loading