- SAP Managed Tags
Did you know JavaScript is object oriented (based on prototypes)? And did you know that there is an instanceof operator available in JavaScript (reminding you of Java etc.)? Of course you do 🙂 Until yesterday I thought it's not worth writing a blog about that, but that has changed after I debugged some UI5 code directly from SAP...
Let's assume we have saved the reference to some UI5 control in a JavaScript variable and want to write code that behaves differently based on the type of control that is referenced. This is what I found in the CRM My Contacts app:
I assume that the
However, the way how the conditions are implemented is error prone. Additionally, I would downgrade the code in terms of maintainability. Why?
First of all, the order of the if's above is very important. For example, if you would switch the 3rd and 1st condition then you will have implemented a nice bug (or feature?), because both sap.m.Input and sap.m.DatePicker inherit from sap.m.InbutBase and thus they have a value property and the corresponging getValue() method. In other words: Your 3rd condition would never be hit for sap.m.DatePicker controls because, instead the first would always hit. So the order is very very important.
Second, the code above has a horrible bug which must be existing for quite a long time - and I will prove that in a few seconds! But first the bug from the users perspective:
We have recently upgraded to the latest My Contacts app from SAP in a Fiori 2 FLP (on-premise):
When starting the app just select one of your contacts from the master list and press the edit button or simply create a new contact. This will lead you to the "Edit/Create" form/view:
Finally press save and see what happens (in case you are aditing an existing contact press save without even changing anything, you'll be very surprised what happens):
As you can see almost all fields are cleared out! And yes, this even happens if you are in edit mode of an existing contact and press save without even changing anything! Hmmm... Even the mandatory field Last Name is empty now, and the backend has accepted the empty mandatory field/value. That does not look correct to me at all! There is not even some kind of frontend validation... I could countinue with some other findings (i.e. related to OData, DataBinding, etc), but let's not go to far...
But why are the fields cleared out? This is a bug in the UI5 code that you see above. So now let's see the bug from a developer's or perspective:
In S4.controller.js the private method _fillNewObject line 799 has the following code n(basically same as above):
So the code is checking if the control has an API method getSelectedKey an then tries to call this method. We assume that the developer expected to have this code executed only in case of sap.m.Select controls (as mentioned above). The implemented check/detection of such controls is error prone in new versions of SAPUI5.
In UI5 version 1.28.43 the control sap.m.Input does not have a property called selectedKey. However, in newer versions of UI5 like 1.44.13 the control sap.m.Input suddenly has a property selectedKey (most probably for the auto suggestions feature…). Because of this all the input fields in the edit view hit the second if statement from the code example above and finally get the new value by calling getSelectedKey(). However, this will return an empty string as the "new value" which is in the end the reason why all input fields are cleared. And all that happens because the developer tries to detect the control by checking for the existence of a certain property - and this is a miserable check! It simply fails! In the chrome dev tools you can see that the (wrong) if block is entered for the firstName input field. This is the first part of the prove I promised you.
So what can we learn from this? Well, don't check for the existence of properties/methods to derive the type/class of a UI5 control. How could this be done better? There are multiple ways, and I prefer this one here:
So using the instanceof operator would have helped us here. To me, the code is better to read, has a better maintainability, and makes the comments useless (therefore I have removed them). Additionally, I have added curly braces 🙂
And now the best part:
The corresponding change of sap.m.Input inside UI5 that breaks the My Contacts app is around for many months now, see the corresponding change on github. It's from 14 Oct 2016, and the issue still exists today in the My Contacts app - this is the second part of the prove I promised you! Some of my colleagues have independently raised the question "How many people other than us actually use the CRM My Contacts app if such an issue has not been fixed/detected yet?" Isn't that a good question? 🙂 Furthermore, I wonder if similar issues exist in the other CRM apps - that causes some headache to me...
Of course, I'm going to create a Note for that and I will let you know about the outcome. I believe every one is affected by this issue - or at least the 5 people who use the My Contacts app 😛
Edit (June 29th, 2017): On June 23rd 2017 a Note has been published which adresses the bug mentioned above
Edit (July 3rd, 2017): I can't believe how the fix in the note mentioned above looks like (Thanks for pointing this out, Daniel). SAP has only changed the the order of the if-statements - not my preference at all:
Cheers,
Nabi
Let's assume we have saved the reference to some UI5 control in a JavaScript variable and want to write code that behaves differently based on the type of control that is referenced. This is what I found in the CRM My Contacts app:
//...
if(oInputField.getDateValue){ //special logic for dates
newValue = oInputField.getValue();
//...
} else if (oInputField.getSelectedKey) //special logic for select field
newValue = oInputField.getSelectedKey();
else if (oInputField.getValue) //special logic for input field
newValue = oInputField.getValue();
else if (oInputField.getSelected) //special logic for radio buttons and check boxes
newValue = oInputField.getSelected();
//...I assume that the
- first condition is actually checking for an sap.m.DatePicker control
- second condition is checking for an sap.m.Select control
- third condition is checking for an sap.m.Input control
- fourth condition is checking for an sap.m.RadioButton or sap.m.CheckBox control
However, the way how the conditions are implemented is error prone. Additionally, I would downgrade the code in terms of maintainability. Why?
First of all, the order of the if's above is very important. For example, if you would switch the 3rd and 1st condition then you will have implemented a nice bug (or feature?), because both sap.m.Input and sap.m.DatePicker inherit from sap.m.InbutBase and thus they have a value property and the corresponging getValue() method. In other words: Your 3rd condition would never be hit for sap.m.DatePicker controls because, instead the first would always hit. So the order is very very important.
Second, the code above has a horrible bug which must be existing for quite a long time - and I will prove that in a few seconds! But first the bug from the users perspective:
We have recently upgraded to the latest My Contacts app from SAP in a Fiori 2 FLP (on-premise):
When starting the app just select one of your contacts from the master list and press the edit button or simply create a new contact. This will lead you to the "Edit/Create" form/view:
Finally press save and see what happens (in case you are aditing an existing contact press save without even changing anything, you'll be very surprised what happens):
As you can see almost all fields are cleared out! And yes, this even happens if you are in edit mode of an existing contact and press save without even changing anything! Hmmm... Even the mandatory field Last Name is empty now, and the backend has accepted the empty mandatory field/value. That does not look correct to me at all! There is not even some kind of frontend validation... I could countinue with some other findings (i.e. related to OData, DataBinding, etc), but let's not go to far...
But why are the fields cleared out? This is a bug in the UI5 code that you see above. So now let's see the bug from a developer's or perspective:
In S4.controller.js the private method _fillNewObject line 799 has the following code n(basically same as above):
So the code is checking if the control has an API method getSelectedKey an then tries to call this method. We assume that the developer expected to have this code executed only in case of sap.m.Select controls (as mentioned above). The implemented check/detection of such controls is error prone in new versions of SAPUI5.
In UI5 version 1.28.43 the control sap.m.Input does not have a property called selectedKey. However, in newer versions of UI5 like 1.44.13 the control sap.m.Input suddenly has a property selectedKey (most probably for the auto suggestions feature…). Because of this all the input fields in the edit view hit the second if statement from the code example above and finally get the new value by calling getSelectedKey(). However, this will return an empty string as the "new value" which is in the end the reason why all input fields are cleared. And all that happens because the developer tries to detect the control by checking for the existence of a certain property - and this is a miserable check! It simply fails! In the chrome dev tools you can see that the (wrong) if block is entered for the firstName input field. This is the first part of the prove I promised you.
So what can we learn from this? Well, don't check for the existence of properties/methods to derive the type/class of a UI5 control. How could this be done better? There are multiple ways, and I prefer this one here:
//...
if(oInputField instanceof sap.m.DatePicker){
newValue = oInputField.getValue();
//...
} else if (oInputField instanceof sap.m.Select) {
newValue = oInputField.getSelectedKey();
} else if (oInputField instanceof sap.m.Input) {
newValue = oInputField.getValue();
} else if (oInputField instanceof sap.m.RadioButton || oInputField instanceof sap.m.CheckBox) {
newValue = oInputField.getSelected();
}
//...So using the instanceof operator would have helped us here. To me, the code is better to read, has a better maintainability, and makes the comments useless (therefore I have removed them). Additionally, I have added curly braces 🙂
And now the best part:
The corresponding change of sap.m.Input inside UI5 that breaks the My Contacts app is around for many months now, see the corresponding change on github. It's from 14 Oct 2016, and the issue still exists today in the My Contacts app - this is the second part of the prove I promised you! Some of my colleagues have independently raised the question "How many people other than us actually use the CRM My Contacts app if such an issue has not been fixed/detected yet?" Isn't that a good question? 🙂 Furthermore, I wonder if similar issues exist in the other CRM apps - that causes some headache to me...
Of course, I'm going to create a Note for that and I will let you know about the outcome. I believe every one is affected by this issue - or at least the 5 people who use the My Contacts app 😛
Edit (June 29th, 2017): On June 23rd 2017 a Note has been published which adresses the bug mentioned above
Edit (July 3rd, 2017): I can't believe how the fix in the note mentioned above looks like (Thanks for pointing this out, Daniel). SAP has only changed the the order of the if-statements - not my preference at all:
Cheers,
Nabi
1 Comment
| User | Count |
|---|---|
| 7 | |
| 6 | |
| 6 | |
| 4 | |
| 4 | |
| 3 | |
| 3 | |
| 3 | |
| 3 | |
| 3 |
