[$70] [Metadata Management] keep entered data if error occurs during entity creating

maxceem commented 5 years ago

This issue is valid for all sections in the metadata: Project Templates, Products Templates, Project Types and so on...

Expected behavior

Entered data should persist in the form when a server-side error happens during some entity creating.

Actual behavior

Entered data is cleared in the form when a server-side error happens during some entity creating.

Steps to reproduce the problem

Open any create page in the metadata section, for example new project template page http://local.topcoder-dev.com:3000/metadata/new-project-template.
Fill the data, but enter some key which already exists in the DB to trigger a server-side error. For example key=app exists.
Click Save button. You would see an error message from the server and form would be cleared, so we have to fill all the data again from the scratch.

Screenshot/screencast

Video: https://monosnap.com/file/9jF1mAcaUirwJKRqcUwDQia8xC0VIn

Screenshot:

--

Environment

OS: all
Browser (w/version): all
User role (client, copilot or manager): admin

vikasrohit commented 5 years ago

Potential candidate for CF

maxceem commented 5 years ago

For testing can use user pshah_manager which doesn't have permission to create any metadata templates. If we try to create a template with such user we would get an error YOU DO NOT HAVE PERMISSIONS TO PERFORM THIS ACTION and form would be cleared. But data in the form should stay when we get an error during template creating.

This issue has to be fixed for all sections:

rahulranjanmca commented 5 years ago

I am not able to assign this to myself?

maxceem commented 5 years ago

Yes members cannot assign themselves in this Bug Bash, please check the rules of this Bug Bash https://www.topcoder.com/challenges/30095031

maxceem commented 5 years ago

@rahulranjanmca I've assigned you as I understand you would like to work on this issue. Let me know if don't want. Anyway, please, check the rules for this Bug Bash, as they are slightly different than usual.

rahulranjanmca commented 5 years ago

@maxceem Yes, I have started setting up the project. Probably first one might take a bit longer. But have started looking into this.

rahulranjanmca commented 5 years ago

@maxceem One Question? Can I setup on windows? Or Only Linux/MacOs needed?

maxceem commented 5 years ago

@rahulranjanmca in general can, but there could be some issues with NPM dependencies. Some of them have to be compiled and might require to install some compilers or libraries in Windows. We don't have any particular instructions one that and they have to be resolved individually.

rahulranjanmca commented 5 years ago

@maxceem I am not able to setup the build. Please unassign it from me.

maxceem commented 5 years ago

Thanks for letting me know @rahulranjanmca.

This issue is now open for pick up.

rashmi73 commented 5 years ago

@maxceem please assign this to me thanks

maxceem commented 5 years ago

@rashmi73 assigned.

r0hit-gupta commented 5 years ago

@maxceem May I take this up?

rashmi73 commented 5 years ago

@maxceem i am active on this PR in sometime

@r0hit-gupta

rashmi73 commented 5 years ago

@maxceem PR ready https://github.com/appirio-tech/connect-app/pull/3145

maxceem commented 5 years ago

@rashmi73 please, have look on the review here https://github.com/appirio-tech/connect-app/pull/3145#pullrequestreview-258586228

maxceem commented 5 years ago

@rashmi73 you have two issues which are waiting for a fix and there are already two days without an update. Do you think you can fix them both during 24 hours, or maybe release one of them?

rashmi73 commented 5 years ago

@maxceem both of them will be fixed in some time

maxceem commented 5 years ago

@rashmi73 we are quite short in time for these issues. When do you think you can finish them? Is 24 hours enough?

rashmi73 commented 5 years ago

@maxceem 4 hours max? are you online for that much time from now?

maxceem commented 5 years ago

@rashmi73 4 hours sounds good. I'm online just 20 minutes more. Will be back tomorrow morning in around 10 hours.

rashmi73 commented 5 years ago

@maxceem I am online and just 15 min more minutes, it is working perfectly for project template, replicating for other fields

maxceem commented 5 years ago

@rashmi73 fix doesn't work for me for project template also, see demo video https://monosnap.com/file/1z70F0UqR4QjJay73a2XDrRxTTsOQH

Could you share a screen video how it works for you?

rashmi73 commented 5 years ago

@maxceem

For http://local.topcoder-dev.com:3000/metadata/new-product-category when it throws error, no response object recevied from backend hence it breaks.

to make my solution work, in PR I am making a null check.

maxceem commented 5 years ago

@rashmi73 tested for http://local.topcoder-dev.com:3000/metadata/new-product-category - the fix works for me.

But for all other sections, the fix doesn't work for me.

rashmi73 commented 5 years ago

@maxceem for plan config, price config and form

backend response on error, is not providing data which was passed in request, whereas for other templates it is perfectly received and i am able to persist data.

in below screenshot, kindly observe "action.payload.config.data", it is a json string, which only has config property and no "key" property is present.ideally it should be recevied as like in other templates

maxceem commented 5 years ago

@rashmi73 got it, the error response for "plan config, price config and form" is empty:

While for others it has an error message:

Please, fix it for

Project Templates - ❌ doesn't work
Product Templates - ❌ doesn't work
Project Types - ❌ doesn't work
Product Categories - ✅already works
Milestone Templates - ❌ doesn't work

And try to fix it for "plan config, price config and form" imagine if we fix the server response.

rashmi73 commented 5 years ago

@maxceem PR updated it will work now, as my previous PR did not contain fix for any hence it was not working . it will work now please check now

rashmi73 commented 5 years ago

@maxceem did i take unnecessary steps for solving this? as category product was already working then i should have copied that?

maxceem commented 5 years ago

as category product was already working then i should have copied that?

it would be great if this issue could be solved the same way is it was for Product Category. As now per testing, the new fix for other sections has some issues, will finish testing in 10 minutes.

maxceem commented 5 years ago

@rashmi73 here is testing after last update https://github.com/appirio-tech/connect-app/pull/3145#pullrequestreview-259253707

maxceem commented 5 years ago

@rashmi73 btw, if you worked on the other issue during night and didn't have a rest, you may take a rest and finish this issue after having a rest, we can wait another day for this issue.

rashmi73 commented 5 years ago

@maxceem to make it work same as product categories we need to then skip showing alert popup.

because product categories has no alert popup of error occured, whereas others have hence values get reseted.

so shall i follow same?

rashmi73 commented 5 years ago

@maxceem thanks, but I did have a rest, will continue working on it. and make a new PR as per your latest comment

maxceem commented 5 years ago

@rashmi73 hmm, I get an alert popup for product categories also, see video https://monosnap.com/file/7DzguqXb5UmbS4xqiW408fg9fVKmhh

Actually, it works perfectly for product categories without explicitly restoring values for this section as you did in your last update for other sections. It would be great if we can implement it the same way for other sections.

rashmi73 commented 5 years ago

@maxceem new PR https://github.com/appirio-tech/connect-app/pull/3154

maxceem commented 5 years ago

Thank you @rashmi73 works great for all cases excpet of Product Templates, please, have a look at the review https://github.com/appirio-tech/connect-app/pull/3154#pullrequestreview-259303294

maxceem commented 5 years ago

Sorry @vikasrohit maybe I've confused you with my comment in the PR https://github.com/appirio-tech/connect-app/pull/3168 but this issue is not yet closed :-)

gets0ul commented 5 years ago

The bug bash challenge is already finished. Is this still open? Can I work on this?

vikasrohit commented 5 years ago

I didn't close it myself, it might be github PR merge that caused this. :)

maxceem commented 5 years ago

@gets0ul it's delayed, but as it is close to being completed, I'm waiting from the current participant to complete it a little bit more.

rashmi73 commented 5 years ago

@maxceem updating in some time

maxceem commented 5 years ago

Ok @rashmi73 I'll wait another 24 hours for the solution. If there is no final solution, I would have to un-assign you.

rashmi73 commented 5 years ago

@maxceem it will work perfectly now. updated.

vikasrohit commented 5 years ago

@maxceem It is working for create screen only, for updates it is still clearing up the changes made by the user.

Also, there is one more issue which we need to take care in next CF:

If there is an error in any of screen, going back to the listing page by navigating the navigation menu, it still shows the error page i.e. it does not clears up the error from Redux state.

Could you please separate issues for these for next release?

maxceem commented 5 years ago

@vikasrohit created separate issues: https://github.com/appirio-tech/connect-app/issues/3179 https://github.com/appirio-tech/connect-app/issues/3180

appirio-tech / connect-app