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 bugs around thesis graduation date #690

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Apr 12, 2021

** Why are these changes being introduced:

  • There is a bug in the thesis model which prevents updating thesis
    records via the admin dashboard. We specify graduation dates in the UI
    via separate month and year fields, but the information gets stored
    internally as a unified date. There is a method to build this date,
    but it only called on record creation - not when the record is updated
  • Additionally, the admin dashboard currently imposes conflicting
    requirements on the graduation_month field, being both a number and
    the name of the month.

** Relevant ticket(s):

** How does this address that need:

  • This updates the thesis model to call combine_gradaution_date every
    time the record gets saved, not just for initial creation.
  • This change also adds unit tests to confirm that this happens on both
    creation and save.
  • Another change is to make the admin dashboard treat months as a text
    string, rather than a number. Ultimately it might need to be a
    dropdown, but we can start here.

** Document any side effects to this change:

None

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

** Why are these changes being introduced:

* There is a bug in the thesis model which prevents updating thesis
  records via the admin dashboard. We specify graduation dates in the UI
  via separate month and year fields, but the information gets stored
  internally as a unified date. There is a method to build this date,
  but it only called on record creation - not when the record is updated
* Additionally, the admin dashboard currently imposes conflicting
  requirements on the graduation_month field, being both a number and
  the name of the month.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/ETD-216

** How does this address that need:

* This updates the thesis model to call combine_gradaution_date every
  time the record gets saved, not just for initial creation.
* This change also adds unit tests to confirm that this happens on both
  creation and save.
* Another change is to make the admin dashboard treat months as a text
  string, rather than a number. Ultimately it might need to be a
  dropdown, but we can start here.

** Document any side effects to this change:

None
@mitlib mitlib temporarily deployed to thesis-submit-pr-690 April 12, 2021 15:44 Inactive
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.476% when pulling bbc58c3 on etd-216-grad-date-bug into a0a1e72 on main.

@JPrevost JPrevost self-assigned this Apr 12, 2021
@matt-bernhardt matt-bernhardt merged commit faf8877 into main Apr 12, 2021
@matt-bernhardt matt-bernhardt deleted the etd-216-grad-date-bug branch April 12, 2021 20:38
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.

4 participants