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

Add fee or salary information to course offer pages #6879

Conversation

avinhurry
Copy link
Contributor

@avinhurry avinhurry commented Apr 28, 2022

Context

In order to remind candidates what the fee or salary is for a course at the point they accept an offer (entering into a "contract"), we should repeat the information from Find within the offer page on Apply.

Changes proposed in this pull request

  • Add the following columns to the TTAPI course sync: fee_details, fee_international, fee_domestic and salary_details

  • Render Fees row which contains fee_international with fee_details (if fee_details is provided) alongside the amount and the fee_domestic on a new line if the international fee is not nil, otherwise only render the fee_domestic with fee details (if provided) on the same line.

  • Render the Salary row if the course is salary or apprenticeship. This is just the salary_details column.

Guidance to review

  • Does the logic make sense, is it what we want?
  • I don't think it's possible for a fee paying course to have neither a fee_domestic and a fee_international so the if statement without the else should be ok, any thoughts or concerns?

Screenshots

image

image

image

Link to Trello card

https://trello.com/c/4vI0TOO1/4364-add-fee-or-salary-information-to-course-offer-pages-on-apply

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • API release notes have been updated if necessary
  • Required environment variables have been updated added to the Azure KeyVault

@avinhurry avinhurry changed the title Remove try call from lat and long Add fee or salary information to course offer pages Apr 29, 2022
@avinhurry avinhurry force-pushed the 4364-add-fee-or-salary-information-to-course-offer-pages-on-apply branch 2 times, most recently from 01641a7 to 7fbf5de Compare May 3, 2022 16:22
@avinhurry avinhurry temporarily deployed to review May 3, 2022 16:37 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 3, 2022 16:41 Destroyed
@avinhurry avinhurry temporarily deployed to review May 3, 2022 17:21 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 3, 2022 17:21 Destroyed
@avinhurry avinhurry marked this pull request as ready for review May 3, 2022 17:24
Copy link
Contributor

@frankieroberto frankieroberto left a comment

Choose a reason for hiding this comment

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

Logic looks good to me. I think some of the free text fields need to rendered with markdown.

Are you planning on plucking the migration out into a separate PR so that it can be shipped first, to avoid errors?

@avinhurry avinhurry temporarily deployed to review May 4, 2022 14:27 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 4, 2022 14:30 Destroyed
@avinhurry
Copy link
Contributor Author

Logic looks good to me. I think some of the free text fields need to rendered with markdown.

Are you planning on plucking the migration out into a separate PR so that it can be shipped first, to avoid errors?

Thanks @frankieroberto, that's good feedback. I've made those changes. Yes we'll merge the migrations PR first in #6915 . Will leave it in this PR too for now so that we can review more easily.

@avinhurry avinhurry temporarily deployed to review May 4, 2022 14:40 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 4, 2022 14:41 Destroyed
@avinhurry avinhurry temporarily deployed to review May 4, 2022 14:58 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 4, 2022 14:58 Destroyed
@avinhurry avinhurry force-pushed the 4364-add-fee-or-salary-information-to-course-offer-pages-on-apply branch from 6a4b6fb to 421c231 Compare May 4, 2022 15:00
@avinhurry avinhurry temporarily deployed to review May 4, 2022 15:05 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 4, 2022 15:05 Destroyed
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2022

Code Coverage

Coverage typeFromTo
↔️Lines covered95.77%95.77%
⬆️Branches covered85.17%85.31%

Line vs. Branch coverage

Branch coverage concerns itself with whether a particular branch of a condition had been executed.
Line coverage is only interested in whether a line of code has been executed.
This comes in handy for measuring one line conditionals.
eg.

          def do_something_with_even_numbers(number)
            return if number.odd?
            ...
        

If all the code in the method was covered you would never know if the guard clause was ever triggered with line coverage as just evaluating the condition marks it as covered.

@avinhurry avinhurry temporarily deployed to review May 4, 2022 15:29 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 4, 2022 15:30 Destroyed
@avinhurry avinhurry force-pushed the 4364-add-fee-or-salary-information-to-course-offer-pages-on-apply branch 2 times, most recently from 20ad8c1 to 84a902e Compare May 5, 2022 12:18
@github-actions github-actions bot temporarily deployed to review-6879 May 5, 2022 12:22 Destroyed
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Code Coverage

Coverage typeFromTo
↔️Lines covered95.78%95.78%
⬇️Branches covered85.42%85.11%

Line vs. Branch coverage

Branch coverage concerns itself with whether a particular branch of a condition had been executed.
Line coverage is only interested in whether a line of code has been executed.
This comes in handy for measuring one line conditionals.
eg.

          def do_something_with_even_numbers(number)
            return if number.odd?
            ...
        

If all the code in the method was covered you would never know if the guard clause was ever triggered with line coverage as just evaluating the condition marks it as covered.

@avinhurry avinhurry temporarily deployed to review May 5, 2022 12:31 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 5, 2022 12:31 Destroyed
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Code Coverage

Coverage typeFromTo
↔️Lines covered95.78%95.78%
⬇️Branches covered85.42%85.32%

Line vs. Branch coverage

Branch coverage concerns itself with whether a particular branch of a condition had been executed.
Line coverage is only interested in whether a line of code has been executed.
This comes in handy for measuring one line conditionals.
eg.

          def do_something_with_even_numbers(number)
            return if number.odd?
            ...
        

If all the code in the method was covered you would never know if the guard clause was ever triggered with line coverage as just evaluating the condition marks it as covered.

@avinhurry avinhurry temporarily deployed to review May 5, 2022 13:42 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 5, 2022 13:45 Destroyed
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Code Coverage

Coverage typeFromTo
↔️Lines covered95.78%95.78%
⬇️Branches covered85.42%85.32%

Line vs. Branch coverage

Branch coverage concerns itself with whether a particular branch of a condition had been executed.
Line coverage is only interested in whether a line of code has been executed.
This comes in handy for measuring one line conditionals.
eg.

          def do_something_with_even_numbers(number)
            return if number.odd?
            ...
        

If all the code in the method was covered you would never know if the guard clause was ever triggered with line coverage as just evaluating the condition marks it as covered.

Copy link
Collaborator

@stevehook stevehook left a comment

Choose a reason for hiding this comment

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

This looks great. I've got one question inline about markdown rendering.

It all worked for me locally. I saw one slight glitch on a course that specified a UK fee but not an International one, there's an empty line between the fee and details:

image

@avinhurry avinhurry temporarily deployed to review May 5, 2022 14:23 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 5, 2022 14:23 Destroyed
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Code Coverage

Coverage typeFromTo
↔️Lines covered95.79%95.79%
⬇️Branches covered85.43%85.31%

Line vs. Branch coverage

Branch coverage concerns itself with whether a particular branch of a condition had been executed.
Line coverage is only interested in whether a line of code has been executed.
This comes in handy for measuring one line conditionals.
eg.

          def do_something_with_even_numbers(number)
            return if number.odd?
            ...
        

If all the code in the method was covered you would never know if the guard clause was ever triggered with line coverage as just evaluating the condition marks it as covered.

@avinhurry avinhurry temporarily deployed to review May 5, 2022 17:14 Inactive
@github-actions github-actions bot temporarily deployed to review-6879 May 5, 2022 17:14 Destroyed
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Code Coverage

Coverage typeFromTo
↔️Lines covered95.79%95.79%
⬇️Branches covered85.43%85.31%

Line vs. Branch coverage

Branch coverage concerns itself with whether a particular branch of a condition had been executed.
Line coverage is only interested in whether a line of code has been executed.
This comes in handy for measuring one line conditionals.
eg.

          def do_something_with_even_numbers(number)
            return if number.odd?
            ...
        

If all the code in the method was covered you would never know if the guard clause was ever triggered with line coverage as just evaluating the condition marks it as covered.

- Add conditionals for rendering each row (in the correct position within the array)

- Add fee_row_value method to allow for further conditionals, i.e only display the international section if populated in the db.
- `fee_details` can be nil, therefore we need to account for this.

- Refactor HTML
@avinhurry avinhurry force-pushed the 4364-add-fee-or-salary-information-to-course-offer-pages-on-apply branch from 5aa46d0 to 1db8d50 Compare May 6, 2022 08:46
@avinhurry avinhurry removed the deploy label May 6, 2022
Copy link
Collaborator

@stevehook stevehook left a comment

Choose a reason for hiding this comment

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

As discussed we think we should keep the markdown processing where it is so the idea of moving it to the course sync only makes sense if we run into performance issues.

@avinhurry avinhurry merged commit 6585685 into main May 6, 2022
@avinhurry avinhurry deleted the 4364-add-fee-or-salary-information-to-course-offer-pages-on-apply branch May 6, 2022 08:57
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.

None yet

3 participants