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

added aamc resource type and rigged it up to terms. #1399

Merged
merged 7 commits into from
May 21, 2016
Merged

added aamc resource type and rigged it up to terms. #1399

merged 7 commits into from
May 21, 2016

Conversation

stopfstedt
Copy link
Member

fixes #566

@stopfstedt
Copy link
Member Author

requires the following command to be run as part of the update process of existing instances:

bin/console doctrine:fixtures:load  --fixtures=src/Ilios/CoreBundle/DataFixtures/ORM/LoadAamcResourceTypeData.php

this will import the AAMC resource types.
failing to do so will not break Ilios, but it will result in missing data points in the Curriculum Inventory export.

@jrjohnson
Copy link
Member

Code reviewed looks good. Maybe we should add the update command to the UPGRADE.md file as part of the PR? - lets discuss how to integrate these types of update notes in our meeting on Monday.

@@ -293,7 +306,7 @@ protected function attachCriteriaToQueryBuilder(QueryBuilder $qb, $criteria, $or

if (is_array($orderBy)) {
foreach ($orderBy as $sort => $order) {
$qb->addOrderBy('t.' . $sort, $order);
$qb->addOrderBy('t.'.$sort, $order);
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk, that should not be. reverting this change.

@jrjohnson jrjohnson assigned stopfstedt and unassigned jrjohnson May 11, 2016
@jrjohnson
Copy link
Member

As discussed in our meeting please add upgrade instructions to UPGRADE.md at the top of the file.

@stopfstedt
Copy link
Member Author

@jrjohnson, please review updated upgrade instructions. thank you.

@stopfstedt stopfstedt assigned jrjohnson and unassigned stopfstedt May 11, 2016
@jrjohnson
Copy link
Member

I'm now wondering if we should indicate that a migration needs to be run each time. I really have no idea how to make this obvious and easy to use. @thecoolestguy any thoughts?

@stopfstedt
Copy link
Member Author

how about this - add a "general upgrade notes" section to the top of this file, outlining the following steps that are assumed to be run each time:

  1. composer install
  2. bin/console doctrine:migrations:migrate.

maybe?

@jrjohnson
Copy link
Member

Yea - that makes sense. That is at the top. Then each version specific step (if required) is at the bottom. i like.

@stopfstedt
Copy link
Member Author

additional schema changes may, or may not, happen here. soon. i'm putting this back on hold.

@homu
Copy link
Contributor

homu commented May 17, 2016

☔ The latest upstream changes (presumably #1421) made this pull request unmergeable. Please resolve the merge conflicts.

@stopfstedt
Copy link
Member Author

ready for review again.

@stopfstedt stopfstedt assigned jrjohnson and unassigned stopfstedt May 18, 2016
@@ -36,7 +36,17 @@ sudo -u apache bin/console cache:clear --env=prod

## Version-specific steps

_to be done_
### Upgrading to Ilios 3.11.0
Copy link
Member

Choose a reason for hiding this comment

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

Should be 3.12.0 which is the next version.

Copy link
Member Author

Choose a reason for hiding this comment

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

handled.

@jrjohnson
Copy link
Member

Tiny tweak to version in README and then this is ready to merge.

@jrjohnson jrjohnson assigned stopfstedt and unassigned jrjohnson May 20, 2016
@stopfstedt stopfstedt assigned jrjohnson and unassigned stopfstedt May 20, 2016
@jrjohnson jrjohnson merged commit 94035b7 into ilios:master May 21, 2016
@jrjohnson jrjohnson mentioned this pull request May 26, 2016
@stopfstedt stopfstedt deleted the 566_track_learning_resources branch June 17, 2016 17:27
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.

Add the ability to track learning resources, per MedBiquitous spec/standards
4 participants