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

SB-868 - Announcement attachment up loader refactoring with injectable service/factory #100

Merged
merged 14 commits into from
Nov 22, 2017

Conversation

nilesh-more
Copy link
Contributor

@nilesh-more nilesh-more commented Nov 21, 2017

@HarishGangula
Copy link
Contributor

closing PR for cleaning repo

angular.module('playerApp').controller('createAnnouncementCtrl', ['$rootScope', '$scope', '$timeout', 'config', 'toasterService', 'announcementService', '$filter', 'uuid4',
function ($rootScope, $scope, $timeout, config, toasterService, announcementService, $filter, uuid4) {
angular.module('playerApp').controller('createAnnouncementCtrl', ['$rootScope', '$scope', '$timeout', 'config', 'toasterService', 'announcementService', '$filter', 'uuid4', 'fileUpload',
function ($rootScope, $scope, $timeout, config, toasterService, announcementService, $filter, uuid4, fileUpload) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$services should be placed before others, please move $filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kochhar resolved
All '$' item should be injected first. cc: @harishgilimi

* @desc - remove selected recipients
* @memberOf Controllers.createAnnouncementCtrl
* @param {object} [item] [current selected item]
*/
createAnn.removeRicipients = function (item) {
Copy link
Contributor

Choose a reason for hiding this comment

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

sp: ricipients > recipients

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kochhar resolved

* @param {Object} option - Option object to invoke controller callback function
* @returns {Callback} Trigger callback function
*/
createFineUploadInstance: function (options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

having the uploader type name in the method does not prevent strong coupling. having the options sent from the controller doesn't prevent abstraction leak.

the factory method should be called when the app is initialised. app-level config options should be passed in and an instance created. this instance is available as a partially configured factory. this factory can then be called with controller specific options to get the uploader instance.

@@ -94,14 +94,14 @@ describe('Controller: createAnnouncementCtrl', function () {
})

it('convert file size into KB / MB', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing expectations here...

spyOn(createAnn, 'convertFileSize').and.callThrough()
createAnn.convertFileSize(0)
spyOn(createAnn, 'getReadableFileSize').and.callThrough()
createAnn.getReadableFileSize(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

... and here.

@nilesh-more nilesh-more reopened this Nov 22, 2017
@loganathan1989 loganathan1989 merged commit c888acc into Sunbird-Ed:release-1.3 Nov 22, 2017
@nilesh-more nilesh-more deleted the refactoring branch January 8, 2018 05:47
rajeevsathish pushed a commit that referenced this pull request Aug 13, 2021
Issue-#SB-25922:Fix-Matrix Instance color on completion
bharathravi-in referenced this pull request in UPHRH-platform/SunbirdEd-portal Jun 5, 2023
UPHRH ISSUE 4899 While editing the batch for PIAA assessment, Udate api is throwing 404 error issue fixed
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