fix feedback to user when something goes wrong in insert superclass/subclass refactor

src/Refactoring-Environment-Tests/RBBrowserEnvironmentTest.class.st

@@ -245,13 +245,14 @@ RBBrowserEnvironmentTest >> testBrowserEnvironment [
 
 { #category : 'tests - environments' }
 RBBrowserEnvironmentTest >> testClassEnvironment [
-
 	| aClassEnvironment |
-	aClassEnvironment := RBClassEnvironment onEnvironment: universalEnvironment classes: (Array with: Object with: Object class).
+	aClassEnvironment := RBClassEnvironment
+		onEnvironment: universalEnvironment
+		classes: (Array with: Object with: Object class).
 	self universalTestFor: aClassEnvironment.
 	self deny: universalEnvironment isClassEnvironment.
 	self assert: aClassEnvironment isClassEnvironment.
-	self assert: aClassEnvironment packages size equals: 1.
+	self assert: (aClassEnvironment packages size) equals: 1.
 	self assert: (aClassEnvironment implementorsOf: #printString) numberSelectors equals: 1.
 
 	self assert: (RBClassEnvironment class: Class) selectors size

src/Refactoring-UI-Tests/ReDriverTest.class.st

@@ -10,5 +10,5 @@ ReDriverTest >> setUpDriver: driver [
 
 	driver previewPresenterClass: StRefactoringPreviewPresenterMock.
 	driver selectDialogForBreakingChanges: StSelectDialogMock new.
-	driver informDialog: StInformDialogMock new.
+	driver informDialog: StInformDialogMock new
 ]

src/Refactoring-UI-Tests/ReDuplicateClassDriverTest.class.st

@@ -80,19 +80,17 @@ ReDuplicateClassDriverTest >> testDuplicateClass [
 { #category : 'tests' }
 ReDuplicateClassDriverTest >> testDuplicateClassInIncompleteModelFails [
 
-	| driver rbClass |
+	| driver |
 	testingEnvironment := RBClassEnvironment classes: { ReClassToBeDuplicated }.
-	driver := ReDuplicateClassDriver className:
-		          ReClassToBeDuplicated name.
+	driver := ReDuplicateClassDriver className: ReClassToBeDuplicated name.
 
 	self setUpMocksOn: driver.
 
+	driver informDialog: StInformDialogMock new.
+	driver application: StApplicationForRefactorDriverMock new.
 	driver scopes: { testingEnvironment }.
-	rbClass := testingEnvironment classes anyOne.
-
-	self
-		should: [ driver runRefactoring ]
-		raise: RBRefactoringError
+	driver runRefactoring.
+	self assert: (driver application shownMessage includesSubstring: 'missing from the model')
 ]
 
 { #category : 'tests' }

src/Refactoring-UI-Tests/ReInsertSubclassBeSiblingDriverTest.class.st

@@ -89,3 +89,49 @@ ReInsertSubclassBeSiblingDriverTest >> testAddSingleSubclassToDifferentPackage [
 
 	self assertSubclassesOf: rbClass
 ]
+
+{ #category : 'tests' }
+ReInsertSubclassBeSiblingDriverTest >> testAddSingleSubclassWithExistingClassNameInformsUser [
+
+	| driver |
+	driver := ReInsertSubclassDriver new.
+
+	self setUpDriver: driver.
+
+	driver application: StApplicationForRefactorDriverMock new.
+
+	driver
+		beSibling;
+		superclass: self superclassToAddSubclass;
+		scopes: { testingEnvironment };
+		subclass: self superclassToAddSubclass name.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring failedApplicabilityPreconditions size equals: 1.
+	self assert: (driver application shownMessage includesSubstring: 'already exists').
+	self assert: self superclassToAddSubclass subclasses size equals: 2
+]
+
+{ #category : 'tests' }
+ReInsertSubclassBeSiblingDriverTest >> testAddSingleSubclassWithInvalidClassNameInformsUser [
+
+	| driver |
+	driver := ReInsertSubclassDriver new.
+
+	self setUpDriver: driver.
+
+	driver application: StApplicationForRefactorDriverMock new.
+
+	driver
+		beSibling;
+		superclass: self superclassToAddSubclass;
+		scopes: { testingEnvironment };
+		subclass: 'noCapital'.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring failedApplicabilityPreconditions size equals: 1.
+	self assert: (driver application shownMessage includesSubstring: 'is not a valid class name').
+	self assert: self superclassToAddSubclass subclasses size equals: 2
+]

src/Refactoring-UI-Tests/ReInsertSubclassDriverTest.class.st

@@ -94,3 +94,47 @@ ReInsertSubclassDriverTest >> testInsertSubclassInADifferentPackage [
 		assert: driver refactoring changes changes first package
 		equals: RBAbstractClass packageName
 ]
+
+{ #category : 'tests' }
+ReInsertSubclassDriverTest >> testInsertSubclassWithExistingClassNameInformsUser [
+
+	| driver |
+	driver := ReInsertSubclassDriver new.
+
+	self setUpDriver: driver.
+
+	driver application: StApplicationForRefactorDriverMock new.
+
+	driver
+		superclass: self superclassToAddSubclass;
+		scopes: { testingEnvironment };
+		subclass: self superclassToAddSubclass name.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring failedApplicabilityPreconditions size equals: 1.
+	self assert: (driver application shownMessage includesSubstring: 'already exists').
+	self assert: self superclassToAddSubclass subclasses size equals: 2
+]
+
+{ #category : 'tests' }
+ReInsertSubclassDriverTest >> testInsertSubclassWithInvalidClassNameInformsUser [
+
+	| driver |
+	driver := ReInsertSubclassDriver new.
+
+	self setUpDriver: driver.
+
+	driver application: StApplicationForRefactorDriverMock new.
+
+	driver
+		superclass: self superclassToAddSubclass;
+		scopes: { testingEnvironment };
+		subclass: 'noCapital'.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring failedApplicabilityPreconditions size equals: 1.
+	self assert: (driver application shownMessage includesSubstring: 'is not a valid class name').
+	self assert: self superclassToAddSubclass subclasses size equals: 2
+]

src/Refactoring-UI-Tests/ReInsertSuperclassDriverTest.class.st

@@ -50,6 +50,71 @@ ReInsertSuperclassDriverTest >> testInsertSuperclass [
 	self assert: rbLeafClass superclass name equals: self newSuperclassName
 ]
 
+{ #category : 'tests' }
+ReInsertSuperclassDriverTest >> testInsertSuperclassWithExistingClassNameInformsUser [
+
+	| driver |
+	driver := ReInsertSuperclassDriver new.
+
+	self setUpDriver: driver.
+
+	driver application: StApplicationForRefactorDriverMock new.
+
+	driver
+		superclass: self theClassToAddASuperclass;
+		scopes: { testingEnvironment };
+		subclass: self theClassToAddASuperclass name.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring failedApplicabilityPreconditions size equals: 1.
+	self assert: (driver application shownMessage includesSubstring: 'already exists').
+	self assert: self theClassToAddASuperclass superclass name equals: #RBRootClassForInsertingSubclasses
+]
+
+{ #category : 'tests' }
+ReInsertSuperclassDriverTest >> testInsertSuperclassWithInvalidClassNameInformsUser [
+
+	| driver |
+	driver := ReInsertSuperclassDriver new.
+	self setUpDriver: driver.
+	driver application: StApplicationForRefactorDriverMock new.
+
+	driver
+		superclass: self theClassToAddASuperclass;
+		scopes: { testingEnvironment };
+		subclass: 'noCapital'.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring failedApplicabilityPreconditions size equals: 1.
+	self assert: (driver application shownMessage includesSubstring: 'is not a valid class name').
+	self assert: self theClassToAddASuperclass superclass name equals: #RBRootClassForInsertingSubclasses
+]
+
+{ #category : 'tests' }
+ReInsertSuperclassDriverTest >> testInsertSuperclassWithoutExplicitParent [
+	| driver rbLeafClass |
+
+	driver := ReInsertSuperclassDriver new.
+
+	self setUpDriver: driver.
+	driver
+		superclass: self theClassToAddASuperclass;
+		scopes: { testingEnvironment };
+		subclass: self newSuperclassName.
+	rbLeafClass := driver superclass.
+
+	self assert: rbLeafClass subclasses size equals: 0.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring changes changes size equals: 2.
+	self assert: rbLeafClass subclasses size equals: 0.
+	self assert: rbLeafClass superclass subclasses size equals: 1.
+	self assert: rbLeafClass superclass name equals: self newSuperclassName
+]
+
 { #category : 'tests' }
 ReInsertSuperclassDriverTest >> theClassToAddASuperclass [
 	"Answer the <Class> which will be superclassed"

src/Refactoring-UI-Tests/StApplicationForRefactorDriverMock.class.st

@@ -0,0 +1,26 @@
+"
+A mock application used by refactoring driver tests.
+
+It records the last message received through #inform: so tests can assert the feedback reported to the user without opening any dialog. It is installed on a driver through the #application: hook.
+"
+Class {
+	#name : 'StApplicationForRefactorDriverMock',
+	#superclass : 'Object',
+	#instVars : [
+		'shownMessage'
+	],
+	#category : 'Refactoring-UI-Tests',
+	#package : 'Refactoring-UI-Tests'
+}
+
+{ #category : 'interaction' }
+StApplicationForRefactorDriverMock >> inform: aString [
+
+	shownMessage := aString
+]
+
+{ #category : 'accessing' }
+StApplicationForRefactorDriverMock >> shownMessage [
+
+	^ shownMessage
+]

src/Refactoring-UI-Tests/StInformDialogMock.class.st

@@ -1,6 +1,9 @@
 Class {
 	#name : 'StInformDialogMock',
 	#superclass : 'Object',
+	#instVars : [
+		'shownMessage'
+	],
 	#category : 'Refactoring-UI-Tests',
 	#package : 'Refactoring-UI-Tests'
 }
@@ -15,13 +18,19 @@ StInformDialogMock >> label: aString [
 { #category : 'accessing' }
 StInformDialogMock >> message: aString [
 
-	^ 'nothing'
+	shownMessage := aString
 ]
 
 { #category : 'api - showing' }
 StInformDialogMock >> openModal [
 ]
 
+{ #category : 'accessing' }
+StInformDialogMock >> shownMessage [
+
+	^ shownMessage
+]
+
 { #category : 'accessing' }
 StInformDialogMock >> text: aString [
 

src/Refactoring-UI-Tests/StRefactoringAddClassPresenterTest.class.st

@@ -13,16 +13,25 @@ Class {
 StRefactoringAddClassPresenterTest >> newPresenter [
 
 	^ StRefactoringAddClassPresenter on: (ReInsertSubclassDriver basicNew
-			   superclass: Object;
-			   yourself)
+		superclass: Object;
+		yourself)
 ]
 
 { #category : 'tests' }
-StRefactoringAddClassPresenterTest >> testCommentTemplateActivationFillsComment [
+StRefactoringAddClassPresenterTest >> testClassNameInputIsRecordedInDriver [
+	| driver |
+
+	driver := presenter driver.
+	presenter textInputPresenter text: 'Object'.
+	self assert: driver subclass equals: 'Object'
+]
 
+{ #category : 'tests' }
+StRefactoringAddClassPresenterTest >> testCommentTemplateActivationFillsComment [
 	| checkbox comment |
-	checkbox := presenter instVarNamed: 'useTemplateCheckPresenter'.
-	comment := presenter instVarNamed: 'commentPresenter'.
+
+	checkbox := presenter useTemplateCheckPresenter.
+	comment := presenter commentPresenter.
 	self assert: comment text isEmpty.
 	checkbox state: true.
 

src/Refactoring-UI/ReInsertSuperclassDriver.class.st

@@ -84,7 +84,6 @@ ReInsertSuperclassDriver >> gatherUserInput [
 
 { #category : 'configuration' }
 ReInsertSuperclassDriver >> instantiateRefactoring [
-	"self ensureSuperclassInModel."
 
 	^ (RBInsertNewClassRefactoring className: self subclass asString)
 		  model: model;
@@ -123,10 +122,9 @@ ReInsertSuperclassDriver >> requestClass [
 ReInsertSuperclassDriver >> scopes: refactoringScopes [
 
 	scopes := refactoringScopes.
-
 	model := self refactoringScopeOn: scopes first.
-	superclass := model classObjectFor: superclass
-
+	superclass := model classObjectFor: superclass.
+	superclassParent ifNil: [ superclassParent := superclass superclass ]
 ]
 
 { #category : 'accessing' }

src/Refactoring-UI/ReInteractionDriver.class.st

@@ -349,14 +349,18 @@ ReInteractionDriver >> requestDialog: aDialog [
 
 { #category : 'execution' }
 ReInteractionDriver >> runRefactoring [
-
-	self configureRefactoring.
-	self gatherUserInput ifFalse: [ ^ self "user abandons" ].
-	self hasFailedApplicabilityPreconditions ifTrue: [ ^ self "applicability failed" ].
-	self hasFailedBehaviorPreservingPreconditions
-		ifTrue: [ self handleBreakingChanges ]
-		ifFalse: [ self applyChanges ].
-
+	"Configures the refactoring, gathers user input and applies the changes."
+
+	[ 
+		self configureRefactoring.
+		self gatherUserInput ifFalse: [ ^ self "user abandons" ].
+		self hasFailedApplicabilityPreconditions ifTrue: [ ^ self "applicability failed" ].
+
+		self hasFailedBehaviorPreservingPreconditions
+			ifTrue: [ self handleBreakingChanges ]
+			ifFalse: [ self applyChanges ] ]
+	on: Error
+	do: [ :e | self inform: 'The refactoring could not be executed: ' , e messageText ]
 ]
 
 { #category : 'accessing' }

src/Refactoring-UI/StRefactoringAddClassPresenter.class.st

@@ -21,6 +21,12 @@ StRefactoringAddClassPresenter class >> defaultPreferredExtent [
 	^ 500 @ 450
 ]
 
+{ #category : 'private - accessing' }
+StRefactoringAddClassPresenter >> commentPresenter [
+
+	^ commentPresenter
+]
+
 { #category : 'accessing' }
 StRefactoringAddClassPresenter >> commentPresenterText [
 
@@ -53,11 +59,8 @@ StRefactoringAddClassPresenter >> defaultLayout [
 StRefactoringAddClassPresenter >> initializeClassNamePresenter [
 
 	textInput := self newTextInput
-		whenTextChangedDo: [ : text | 
-			(Smalltalk globals hasClassNamed: text asSymbol)
-				ifTrue: [ self inform: 'Class ' , text asString , ' already exists' ]
-				ifFalse: [ driver subclass: self newClassName ] ];
-		yourself.
+		whenTextChangedDo: [ :text | driver subclass: self newClassName ];
+		yourself
 ]
 
 { #category : 'initialization' }
@@ -135,6 +138,12 @@ StRefactoringAddClassPresenter >> updateTagItems: item [
 	tagPresenter selectIndex: 1
 ]
 
+{ #category : 'private - accessing' }
+StRefactoringAddClassPresenter >> useTemplateCheckPresenter [
+
+	^ useTemplateCheckPresenter
+]
+
 { #category : 'accessing' }
 StRefactoringAddClassPresenter >> windowIcon [