Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Add abstract controller unit tests and translations #900

Merged
merged 5 commits into from
Dec 21, 2016

Conversation

mkly
Copy link
Contributor

@mkly mkly commented Dec 20, 2016

Changes proposed in this pull request:

  • Add unit tests for controller abstract-delete-controller
  • Add dependency(devDependency) for ember-sinon-qunit for use in
    stubbing methods for proper test coverage
  • Update documentation with @todo about possible unintended behavior
    of the sortByKey action
    The tests in this commit preserve the behavior as implemented. It is
    possible that sortByKey should also set nextStartKey and startKey
    to null as well.
  • Add translations for _notifyReportError in
    abstract-report-controller
  • Add unit test for _notifyReportError
  • Add translations for _setReportTitle in abstract-report-controller
  • Add unit tests for _setReportTitle with translations
  • Add unit tests for abstract-report-controller
  • Add unit tests from abstract-paged-controller

Finally needed the sinon devDependency here. I'll circle back after this is merged and pull the timekeeper stuff out.

Not 100% sure of where I should be putting the translations in the locale file. Just making a best guess so definitely let me know if I should move them to a better spot.

This is all in one big PR because of later tests needing sinon as well. I tried to keep the commits clean, so when reviewing it is probably easiest for you to step through the commit diffs if you are looking to get a closer look at the changes.

cc @HospitalRun/core-maintainers

mkly added 5 commits December 20, 2016 14:50
- Add unit tests for controller abstract-delete-controller
- Add dependency(devDependency) for ember-sinon-qunit for use in
  stubbing methods for proper test coverage
- Add unit tests from abstract-paged-controller
- Update documentation with `@todo` about possible unintended behavior
  of the `sortByKey` action

The tests in this commit preserve the behavior as implemented. It is
possible that `sortByKey` should also set `nextStartKey` and `startKey`
to `null` as well.
- Add unit tests for abstract-report-controller
- Add translations for `_notifyReportError` in
  abstract-report-controller
- Add unit test for `_notifyReportError`
- Add translations for `_setReportTitle` in abstract-report-controller
- Add unit tests for `_setReportTitle` with translations
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mkly! Looks good to me.
I think the translation locations are fine, but that file needs some more organization (eg we are not always using a consistent hierarchy), but I don't think that should be cleaned up as part of this PR.

In regards to nextStartKey and firstKey that looks like a bug to me, because those values could potentially change on sort.

@jkleinsc jkleinsc merged commit ca37bf7 into HospitalRun:master Dec 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants