appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard

[Feature]-[4000]:Autosave for new datasources needs improvements

Open areyabhishek opened this issue 4 years ago • 19 comments

Description

In Appsmith we try to autosave everywhere. Today when a user clicks on a new datasource, it creates new named resource even though the user might have just been exploring.

Solution

We should not save the datasource if there was nothing typed inside the form. It should just disappear because the user was exploring and not actually creating a new datasource.

areyabhishek avatar May 08 '21 10:05 areyabhishek

Can i work on this issue?

mandvisingh avatar Oct 04 '21 12:10 mandvisingh

@mandvisingh go for it! Do read our contribution guidelines and raise a PR within 4 days :)

Nikhil-Nandagopal avatar Oct 04 '21 12:10 Nikhil-Nandagopal

Can you point me to where the code for the autosaver is at? I tried searching through the code but couldn't quickly find it. If it's as simple as wrapping it in an if value!='' statement, I am happy to take this one and can do it within 4 days!

brittanyjoiner15 avatar Oct 11 '21 21:10 brittanyjoiner15

@brittanyjoiner15 Unfortunately, this issue involves a lot of refactoring and doesn't meet our criteria of what qualifies as a good first issue. This issue will be picked up by one of our internal team members.

arunvjn avatar Oct 12 '21 06:10 arunvjn

@arunvjn ah, i see! good luck!

brittanyjoiner15 avatar Oct 12 '21 14:10 brittanyjoiner15

datasources created, 6m: 12000 assume 1/3 of users go away from the configuration page with unsaved changes: 4000

Stats

Stat Values
Reach      4000
Effort (months)  <>

rohan-arthur avatar Jun 05 '22 06:06 rohan-arthur

datasources created, 6m: 12000 assume 1/3 of users go away from the configuration page with unsaved changes: 4000

Stats

Stat Values
Reach      4000
Effort (months)  0.5

ayushpahwa avatar Jun 07 '22 15:06 ayushpahwa

design link: https://www.notion.so/appsmith/Autosave-vs-Manual-save-in-Datasource-fb96a01c3df74a41a4bd0c63398188fe

rohan-arthur avatar Jun 08 '22 09:06 rohan-arthur

User flows

Create Mode Intends to create, intends to save

  1. I click on new datasource + and select the plugin to see the create datasource form
  2. I should be able to enter the fields I want, then click save, so that the datasource is created

Intends to create, does not intend to save In this case, I should be prompted to save

  1. I click on new datasource + and select the plugin to see the create datasource form
  2. I click the Back or Cancel button with or without making any changes to the form
  3. I should be prompted to ensure whether I want to save my changes or not, so that: a. yes: the datasource is saved b. no: the datasource is not saved, I am taken to the previous page from where I navigated here also discussed here and here

Does not intend to create

  1. I click on new datasource + and select the plugin to see the create datasource form
  2. . I should be able to use the Back or Cancel action, so that by clicking this, I can go back to the previous page from where I navigated here
  3. the datasource should not get created

Edit Mode Intends to view only

  1. When I click on Edit for a datasource, I should see the datasource form in edit mode
  2. The Save button should be disabled by default
  3. I should be able to navigate back to the previous page using Back or Cancel

Intends to make changes and save

  1. When I click on Edit for a datasource, I should see the datasource form in edit mode
  2. The Save button should be disabled by default
  3. I should be able to make changes, upon which the Save button should get enabled
  4. When I click save, my latest changes should get saved to the datasource

Intends to make changes and not save

  1. When I click on Edit for a datasource, I should see the datasource form in edit mode
  2. The Save button should be disabled by default
  3. Now if I hit Back or Cancel, I should be prompted to ensure whether I want to save my changes or not, so that: a. yes: the datasource is saved b. no: the datasource is not saved

rohan-arthur avatar Oct 05 '22 05:10 rohan-arthur

@rohan-arthur I have a couple of questions regarding this functionality:

  • so currently suppose if I create a new datasource and instead of saving that datasource, user reloads the page, we get the same datasource page in view mode, which user can then edit, but if we are planning to create datasource only when user clicks on save button, then what would happen if user reloads the page in such case before actually saving the datasource?
  • We have delete and test buttons along with save button in datasource configuration form, but if we are planning to create datasource only on the click of save button, then what would happen if user clicks on delete/test button before clicking on save and creating the datasource? In this case should we hide/disable these buttons?

CC: @AmanAgarwal041

sneha122 avatar Oct 10 '22 06:10 sneha122

@sneha122 :

  1. close tab, refresh should be handled similarly but let's keep it out of scope for this issue - since its incidence is lesser and it is complicated to implement. For now, the changes should simply not be saved if I close or refresh the tab. If on refresh, the default behaviour is retaining the user's changes, but they are not saved yet - even so, let's go with it.
  2. Test should be possible without save, so there need not be any change.
  3. Delete, you're right - this does not make sense. Can we disable the delete if no save has happened yet in create mode?

rohan-arthur avatar Oct 10 '22 08:10 rohan-arthur

@rohan-arthur @sneha122

  1. When creating a datasource, we should not switch to view mode after clicking the save button; instead, we should stay in that mode and enable the test button. By doing this, the user can test the datasource with just one click.
  2. When a user creates a datasource, we are aware of the path. In order to keep the last altered state when a user refreshes the page, we may either build a new temporary datasource on the client or save the updated data in local storage in the same way that it was previously saved on the server.

AmanAgarwal041 avatar Oct 10 '22 08:10 AmanAgarwal041

@AmanAgarwal041 that doesn't sound like a good UX because the feedback on saving the datasource is not very strong. When a user saves the datasource, we can automatically test it and assign it a state as

  • Successfully connected
  • Error in connecting
  • Not testable

Nikhil-Nandagopal avatar Oct 10 '22 08:10 Nikhil-Nandagopal

@AmanAgarwal041 Moreover currently when we save the datasource it takes us back to active datasource list, and not in the datasource config viewMode, do we need to alter that behaviour as well?

sneha122 avatar Oct 10 '22 09:10 sneha122

@AmanAgarwal041 Moreover currently when we save the datasource it takes us back to active datasource list, and not in the datasource config viewMode, do we need to alter that behaviour as well? @rohan-arthur Any thoughts here ?

AmanAgarwal041 avatar Oct 10 '22 09:10 AmanAgarwal041

@sneha122 @AmanAgarwal041 after saving, ideally I should see the ds that I just saved. If this is not happening, please create a separate issue for this (looks like this issue has large enough scope already)

rohan-arthur avatar Oct 10 '22 09:10 rohan-arthur

@sneha122 @rohan-arthur @AmanAgarwal041

  • As per my understanding the data in the form is saved in local storage and it persists till the point when the session is reloaded or killed (when I refresh the tab or kill the tab). Aside from these two options even if i move from the data source page to canvas or anywhere else and I come back to edit the data source the value still persists. Since the value persists it conveys the message that it's saved and there is no indicator if it's in local storage or saved on the server. Reference Video.
    Keeping this in mind, are we still going to continue saving the value in local storage if so then we need some indicator to covey this or are we going to destroy the value if the user exists the configuration form without saving? IMO we should not have values persists locally when the user has moved away from the data source configuration form, this way we explicitly tell the users to save the values before exiting.

  • Agreed with @rohan-arthur that the delete option should be disabled when the data source is not saved.

  • @rohan-arthur after saving the data source it should lead back to the review page of the datasource created where we provide the option to create a new query/API. We should pick this up with this since it's part of the core experience.

parth-appsmith avatar Oct 10 '22 10:10 parth-appsmith

As per the discussion, we would go with following approach for datasource autosave:

  • Delete button will be disabled until user saves the datasource for the first time
  • Test button will remain enabled, as user should be able to test the datasource without actually having to save the datasource
  • Whenever user moves from create datasource/edit datasource configuration page without saving the data, then they would be shown warning popup as to save the data or dont save the data. If dont save is clicked, all temporary datasource data/edits made by user to existing datasource will be lost.

sneha122 avatar Oct 11 '22 13:10 sneha122

@AmanAgarwal041 that doesn't sound like a good UX because the feedback on saving the datasource is not very strong. When a user saves the datasource, we can automatically test it and assign it a state as

  • Successfully connected
  • Error in connecting
  • Not testable

@Nikhil-Nandagopal I would be creating a separate ticket for this CC: @rohan-arthur @AmanAgarwal041 @parth-appsmith

sneha122 avatar Oct 11 '22 13:10 sneha122

@rohan-arthur Here are a couple of the points not taken into account/increased the scope of this ticket:

  • Save as datasource for REST and GraphQL API flow
  • After save earlier it used to go active DS list, but now it needs to stay in DS config page with view mode, Although we discussed that we can take this change later on. since in the discard popup designs also, on save it was going on view mode ds config page, I made that change
  • When we import applications, we have datasource reconnect popup, where we need to configure datasources again.

sneha122 avatar Nov 02 '22 14:11 sneha122

While checking the failed cypress tests for datasource autosave improvements, I came across one test in globalsearch_spec.js, which attempts to navigate to same datasource while its creation, following is the video attached: With new implementation, since we save datasource only on save button click, what should we do in such case? cc: @rohan-arthur @sumitsum @AmanAgarwal041 @parth-appsmith

https://user-images.githubusercontent.com/30018882/202695918-ba64e1b8-1378-43e7-a99c-f537947afcb9.mov

sneha122 avatar Nov 18 '22 11:11 sneha122

While checking the failed cypress tests for datasource autosave improvements, I came across one test in globalsearch_spec.js, which attempts to navigate to same datasource while its creation, following is the video attached: With new implementation, since we save datasource only on save button click, what should we do in such case? cc: @rohan-arthur @sumitsum @AmanAgarwal041 @parth-appsmith

Screen.Recording.2022-11-18.at.5.01.55.PM.mov

I believe that the global menu should not list the same datasource because the new datasource has not yet been created without pressing the save button. Can we anticipate this behaviour from the test suite?

AmanAgarwal041 avatar Nov 21 '22 08:11 AmanAgarwal041

Yes we can fix this in a way that datasource itself wont be shown in the list, because it has not been created yet. But I think there would be issues in replicating this behaviour in test suite because we do not have tests for google sheets authentication yet.

sneha122 avatar Nov 21 '22 10:11 sneha122

@sneha122 sorry I couldn't understand this part - But I think there would be issues in replicating this behaviour in test suite because we do not have tests for google sheets authentication yet. Can you please explain this ?

sumitsum avatar Nov 21 '22 11:11 sumitsum

@AmanAgarwal041 sorry I couldn't understand this, can you please explain once ? Can we anticipate this behaviour from the test suite?

sumitsum avatar Nov 21 '22 11:11 sumitsum