issue 3149 fix

[rashmi73]

No description provided.

[maxceem]

Thanks, @rashmi73

Most cases are fixed now. But some cases are still there:

• If in the question Devices we select all devices, and after unselect as it was before, the button Save changes would stay enabled, see demo video: https://monosnap.com/file/W2S5i8ZfZ94f7GLuZfpSubc5BqWE0m

[rashmi73]

@maxceem updating in sometime

[rashmi73]

@maxceem this conditon is covered after a lot of analyzing

PR updated.

Also, can you please help me in understanding
1.what preparedconditons and conditions property inside subsection array does exactly?

2. though i am aware it is to check validity of question based on conditons.

because wizardhelper.js has lot of utils, which are required to understand the flow, how properties are manipulated.

but any such document which is internally shared with your team ? which can help understand object tree well?as then it would be lot easier to understand code.

[rashmi73]

@maxceem this is updated long back. you can test it

[maxceem]

Thanks for update @rashmi73.

Project Templates Overview

Let me provide more details on how the forms work and answer your questions.

In the Connect app customers can create projects. Each project may have some special type and need different set of questions to ask the customer. So when a customer creates a project https://connect.topcoder-dev.com/new-project/ he can choose some type of project and he would get some different form with a different set of questions. Each such form is defined by Project Templates JSON which you can see here http://local.topcoder-dev.com:3000/metadata/projectTemplates. And here is an example of one project template JSON https://codeshare.io/GABXjr.

In the project templates, we can define different type of fields to show in the form. Also, we can add conditions so we can hide/show some fields depend on the value in other fields.

1.what preparedconditons and conditions property inside subsection array does exactly?

So the condition properties have some expression which is calculated based on what values user choose in other fields, to decide if we should hide or show some subsection.

preparedconditons are kind of variables for conditions. To not write long conditions we prepare some variables in preparedconditons and after use them in conditions.

This was for you to have some general view about project templates though I think this issue may be fixed without relying on the details provided above.

Issue implementation details

The main reason for the issue is because we can show/hide them. So when we hide some filed, we don't have it's value at all in the project details - it's undefined. But when we show it, the field would be shown, but if we don't choose any value, then value would be empty string, empty object or an empty array. So the difference is if field is visible at the moment or no, but still it doesn't have value.

You found the root of the issue correctly, though the fix should be more general. As I've mentioned above we have many types of templates with a different set of questions and field names. The suggested fix would work for the very specific situation and will fail for all other templates.

We should fix the way we compare project data to take care of situations where field can have undefined or empty string or empty array or an empty object.

So we can update the code at this line https://github.com/appirio-tech/connect-app/pull/3150/files#diff-a52fca6a10cebc946297b5eb2dca6533R648
instead of isEqual we should compare taking into account the logic above. I guess we can use isEqualWith and provide a custom function to compare as a third argument see lodash documentation - this is just a suggestion way to handle it.

In our custom comparison function we should treat equally:

• undefined == ""
• undefined == {}
• undefined == []

The only exception for utm.code - we can remove it from comparison as we are not able to change it actually.

So it would handle these cases:

To debug how currently comparison is done you can update code to this one, and check what data it outputs to browser console when you update project values:

if (!_.isEqualWith(clearUpdatedProject, clearUpdatedNonDirtyProject, (objValue, othValue, key) => {
      console.warn(key, objValue, othValue )
    })) {
      updatedProject.isDirty = true
    }

So you can see how isEqualWith compares properties one by one.

Regarding the issue in this comment #3150 (review) I'm not sure what exactly should be fixed for this one but we should not implement the specific fix for this issue:

We should fix the comparison function in general so it correctly compares two project objects: clearUpdatedNonDirtyProject and clearUpdatedProject as the template logic described above.

As I see that fixing this issue is more complicated than it looks from the beginning the prize for this issue can be raised to $150. But the fix should be general as described above. Other words we should not use names of the specific field like appDefinition , features, notes, targetDevices and so on, or specific field values like web-browser.

Please, let me know if you would like to continue to work on this issue with updated details.

[rashmi73]

@maxceem
Thanks for detailed explanation, i got much idea about it now.

Yes I will take this, as I feel it can be solved by me, but the modification you mentioned for https://github.com/appirio-tech/connect-app/pull/3150/files#diff-a52fca6a10cebc946297b5eb2dca6533R648

can I go with different approach and do modification in "_.mergeWith" callback function, as this is util which forms "updatedProject" object, so better i apply general check here itself and make perfectly "updatedProject", so that isEqualCheck needs no modification.

[maxceem]

@rashmi73

Yes I will take this, as I feel it can be solved by me

Sure, but let's set a deadline for this task for 48 hours as it already took too long. And this issue is quite serious and should be fixed asap. So if there is no final fix for 48 hours, I'll have to assign you and solve it some other way.

can I go with different approach and do modification in "_.mergeWith" callback function, as this is util which forms "updatedProject" object, so better i apply general check here itself and make perfectly "updatedProject", so that isEqualCheck needs no modification.

I'm not sure where you want to use mergeWith. But I'm fine with any fix if it's general and works for any form data not only for this particular form.

I just want to note, if you want to use mergeWith inside wizardHelpers methods, you can fix the current form data which we edit. But you still have to fix data which comes from the server, as on the server we already have forms with any kind of data. Also, note, that methods inside wizardHelpers are kind of sensetive and easy to broke, as they are also used for project creation forms and logic here is quite complicated.

So IMHO its easier and safer to fix comparison at this line https://github.com/appirio-tech/connect-app/pull/3150/files#diff-a52fca6a10cebc946297b5eb2dca6533R648. But if you feel confident about your approach I'm also fine with it as long as it works for all forms data and doesn't break existent functionality.

[rashmi73]

@maxceem can i get something for my efforts till now in finding root cause?

also many things are already given as hint by me(though it is hardcoded), but now i am in doubt whether i can solve it completely because even if i go with your approach
1.how to determine whether the property needs to be omitted?
2. by looking at prepared conditions ? or conditions?

Hence I wish to opt out from here as I do not wish to waste time if I could not solve it.

Thanks

[rashmi73]

@maxceem i guess i have a solution, working on it

[maxceem]

@rashmi73 as there is no solution yet and we don't have time anymore to give it a try so I have to un-assigning you.

@maxceem can i get something for my efforts till now in finding root cause?
also many things are already given as hint by me(though it is hardcoded)

Generally, we only accept and grant payments for solved issues. If the issue is not solved it cannot be paid.
As your finding are still valuable, as an exception I may process half payment for the initial issue $30. I understand that it may be not covers your efforts, so I would recommend to give up the issue next time if you feel that you cannot find a solution in a reasonable time to not spend your efforts without a reward.

Thanks.