Skip to content

Conversation

@gets0ul
Copy link
Contributor

@gets0ul gets0ul commented Jul 14, 2019

@maxceem maxceem self-requested a review July 15, 2019 08:22
Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks @gets0ul.

As has been mentioned in the comment to the previous PR #3150 (review):

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.

But in proposed implemenation there is some logic which is bound for particular templates as it uses references to budgetDetails, deliverables, dev, development, data-science, dataScience, deploymentTargets, progressiveResponsive and so on.

All these values are just some particular data in one project template. It could be different values and our intention here is to compare any data without binding to a particular project template.

Can we have a general fix for this issue, without binding to the particular project data?

@gets0ul
Copy link
Contributor Author

gets0ul commented Jul 15, 2019

@maxceem
Thanks for the feedback.
I'll let you know if I can comeback with a general fix.

@gets0ul
Copy link
Contributor Author

gets0ul commented Jul 15, 2019

@maxceem
We can remove out all the changes with specified fields, and leave only general part from my PR.
It will work.
The only problem is the default initial value of an option when they are selected and deselected.

From #3150 (review)

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.

It's easy if the value were empty string, empty object or an empty array. It's already taken care by general fix of my code.

The problem here if the value is none of above.
For example, https://connect.topcoder-dev.com/projects/8186/scope
The type of budget. The current value is "Design".
If we add "Development", it will set the Development Units field development.qty = 1 in budgetDetails
"Save Changes" button is enabled.
Now if we deselect the "Development", it won't disable the "Save Changes" button again as the "development.qty = 1" remains there in budgetDetails. Need to manually check if budgetDetails.deliverables array no longer contain 'dev'.

Same as https://connect.topcoder-dev.com/projects/8197/scope
If we change the "Devices" to "Web Browser" we need to choose the "Web App Type" from radiobutton group which will set the progressiveResponse field in the dirty project.
If we change our mind and revert back the "Devices" to "Mobile", the field progressiveResponse is still there so it stays dirty.

For the first example, it's easier to solve as it requires the discipline of the app user.
Project 8186. we can clear the Development Units field and deselect Development.
The "Save Changes" button will be disabled again.

But it is not that easy for the 2nd example as it came from radio button value and there is no way to reset/deselect it.

Having said that, I will try to look if a general fix that covering all cases is possible.

@gets0ul
Copy link
Contributor Author

gets0ul commented Jul 15, 2019

@maxceem
PR updated without using hardcoded fields.
Thanks.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@gets0ul that's a great update without using hardcoded values.

I've tested the form and can see that it now works for some cases with this fix. Though there are some cases when button Save Changes doesn't get enabled when we change something, see demo video:

  1. When we select Deployment in Work needed.
  2. When we select Web Browser in Devices.


isChanged() {
return !!this.props.project.isDirty
return !!this.state.isProjectDirty
Copy link
Collaborator

@maxceem maxceem Jul 16, 2019

Choose a reason for hiding this comment

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

@gets0ul I guess when we are making this change, we don't use isDirty which we update inside Redux store, so I have a couple of questions:

  1. Should we still have and update isDirty in the Redux store?
  2. Do you think it's safe to use isProjectDirty instead of isDirty from the Redux store?

Copy link
Contributor Author

@gets0ul gets0ul Jul 16, 2019

Choose a reason for hiding this comment

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

isDirty from Redux is still used.
That is where the initial value of isProjectDirty come from.

Both will always have same value most of the time.
The only time they can be different is for the cases I already mentioned here #3175 (comment)

When a node is updated and the hidden value is still there.
When an option is selected and the default value is set.

isDirty will stay true when that option is deselected because of that hidden default value.
That's the only time, the value of isProjectDirty will be different.

I think it is safe as we have already been using the state isProjectDirty when creating SpecSection component to render the questions.

@gets0ul
Copy link
Contributor Author

gets0ul commented Jul 16, 2019

@maxceem

That's the correct behavior.
Selecting Deployment requires you to choose the Deployment Targets
The same case as Web Browser with the 'Web App Type`

The save button won't be enabled until we enter the value,

@maxceem
Copy link
Collaborator

maxceem commented Jul 17, 2019

Sorry for the delay with review @gets0ul. Today we are releasing the new version of Connect app. Will review this PR tomorrow.

// project's name might contain ampersand
if (key === 'name') {
return _.escape(srcValue)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gets0ul I'm not sure what is the reason for adding this code, is there any situation when it helps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxceem

It is for the case like this https://connect.topcoder-dev.com/projects/7977/scope
when a project contains ampersand on it's name.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@gets0ul this fix works good to me.

I've just added one comment above to clarify the code.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks, @gets0ul

I did testing several times because logic is quite tricky here but couldn't find any issues.

@maxceem maxceem changed the base branch from cf18 to dev July 22, 2019 03:24
@maxceem maxceem merged commit 5dff685 into topcoder-archive:dev Jul 22, 2019
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.

2 participants