parse-community / parse-dashboard

A dashboard for managing Parse Server
https://parseplatform.org
Other
3.73k stars 1.38k forks source link

feat: Add Cloud Config change history to roll back to previous values #2554

Closed iMac7 closed 1 month ago

iMac7 commented 1 month ago

New Pull Request Checklist

Issue Description

Adds recent config history using localstorage for better undo functionality

Closes: #2339

Approach

Every time the user updates a value in the config, it is saved to localstorage under the key configHistory . For each parameter in the config, an array of values is stored in descending order to show the latest value first.

TODOs before merging

parse-github-assistant[bot] commented 1 month ago

Thanks for opening this pull request!

parse-github-assistant[bot] commented 1 month ago

I will reformat the title to use the proper commit message syntax.

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51593

:watch: Updated May 11, 2024, 01:46 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

mtrezza commented 1 month ago

Thanks for picking this up! I think we need a different way to display the history. Below are some issues I found.

Example 1:

image

Example 2:

image

Example 3:

image

Example 4:

image

Example 5:

image

My suggestion to solve these issues is a much simpler implementation:

iMac7 commented 1 month ago
  • Replace the individual buttons of previous values with a drop-down that shows a list of timestamps (date + time in hours, minutes, seconds) of when the previous values were saved. When selecting a timestamp in the drop-down, update the param value field so the user can inspect the previous value before restoring it by clicking on the "save" button.

My latest PR fixes the various UI/logic issues that you outlined, so I feel we don't need timestamps to prevent the dialog from breaking. I can still add if deemed necessary after a re-review

  • How many previous values are stored, is there a limit in counts or are there any storage size limitations? Should the count of previous values be configurable as a dashboard option?

Not sure so I used an opinionated value of 10 history entries per parameter to be very light on the user's storage. I don't think we need that much data considering this feature is mostly about tracking the most recent changes :thinking:

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51601

:watch: Updated May 11, 2024, 21:58 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

mtrezza commented 1 month ago

image

A scrollable list of buttons is an unconventional UI choice. Drop-down lists are traditionally used for this. We have the timestamp information, which is generally the primary information in a "rollback" feature, so we should display it. In a complex environment, param changes influence metrics over time, so time correlation between events is key.

I understand that showing previous values in button labels can make it easier to quickly choose a value to roll back to, but that works only for short values (limited button label text length), and it is a duplicate functionality, because the full value is displayed before saving anyway.

I would still suggest to change this to a drop-down list with timestamps. In future improvements we can think about convenience features such as quick value preview, version comparison (diff highlighting, etc) and how an intuitive UI should look like.

Just an idea off the top of my head: if you would like to implement a quick value preview with this PR, why not add an "i" (info) icon on the side of the timestamp and show the value in a tool-tip when hovering with the mouse over it. This allows to display more of the previous value than the limited button label text length, so it would provide an even better value preview functionality than the list of buttons. Another option could be a button to fold-open the value beneath the timestamp, but that sounds more complicated to implement, maybe better to avoid for now.

Not sure so I used an opinionated value of 10 history entries per parameter to be very light on the user's storage

I'm not sure whether Parse Server enforces any specific limit. All config params are stored in a single document. The max document size is 16MB. 16MB x 10 = 160MB max storage needs. Not sure what limits there are on the browser side. We can leave it as for now it and react to any feedback later on.

iMac7 commented 1 month ago

Just an idea off the top of my head: if you would like to implement a quick value preview with this PR, why not add an "i" (info) icon on the side of the timestamp and show the value in a tool-tip when hovering with the mouse over it. This allows to display more of the previous value than the limited button label text length, so it would provide an even better value preview functionality than the list of buttons. Another option could be a button to fold-open the value beneath the timestamp, but that sounds more complicated to implement, maybe better to avoid for now.

overflow: scroll does not work well with the tooltip and forcefully cuts off text. The second option was actually easier XD

Probably some style adjustments I can't find the info icon , where do I check the list of all available icons?

mtrezza commented 1 month ago

The icons are in a collective SVG I believe. Just look at any existing dialog that has an icon, it should point you to where that comes from.

image

Could you please:

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51610

:watch: Updated May 13, 2024, 02:09 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51729

:watch: Updated May 14, 2024, 22:42 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

mtrezza commented 1 month ago

Thanks for the adaptation, there seem to be a few layout issues regarding the tooltip (covers timestamp, appears without delay, no border, etc). This seems to be a custom built tooltip, but a normal bowser-native tooltip would be better, because the browser deals with the layout and we don't have to re-invent the wheel.

image image

To go ahead and get this feature merged, I suggest to:

iMac7 commented 1 month ago

Thanks for the adaptation, there seem to be a few layout issues regarding the tooltip (covers timestamp, appears without delay, no border, etc). This seems to be a custom built tooltip, but a normal bowser-native tooltip would be better, because the browser deals with the layout and we don't have to re-invent the wheel.

I tried implementing this, but this feature which connects a tooltip to its parent is only available in chrome 125 which itself seems to be in beta, even the anchor examples in that page don't run XD . Any sort of tooltip seems to be problematic to implement in one way or the other so I have removed the preview.

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51784

:watch: Updated May 15, 2024, 17:38 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

mtrezza commented 1 month ago

Yes! - I think we're almost done here, just some nits I found:

iMac7 commented 1 month ago
  • Date: creating a new date param crash the dashboard, and it crashes when loading the Config param page

Interesting one, When I create a date type param, it is of type Date and looks like a normal iso date string ("2024-05-16T10:36:58.563Z"), which is how I save it in localStorage. When I refresh the page, that same Date type is replaced by an Object type with a different structure ({"__type":"Date","iso":"2024-05-01T11:44:00.000Z"}), which is what breaks the page. If you don't refresh the page after creating the param, the Date type remains.

Is this intended behavior? Which variation of date should I use?

mtrezza commented 1 month ago

Not sure, why not save the date as JS Date object in local storage, and when selecting a previous value set the value as if the user had entered it?

iMac7 commented 1 month ago

Config history is now available for date values, but only after refreshing the page once the param is created to account for that edge case. However, the values are properly saved even if the user does not refresh, the buttons are just not "click to apply" yet

mtrezza commented 1 month ago

What is the edge case? Refreshing the dashboard is actually a big thing because it's running locally once loaded.

iMac7 commented 1 month ago

What is the edge case? Refreshing the dashboard is actually a big thing because it's running locally once loaded.

The type and structure you get when you create a date is not the same type you get when you refresh, it turns from date to object for some reason. See the type is Object below

IMG20240516174414

mtrezza commented 1 month ago

Not sure I follow, could you explain step-by-step?

The obj you see looks like the way Parse stores a date on a Parse Object, so it may do the same for the config param.

iMac7 commented 1 month ago

https://github.com/parse-community/parse-dashboard/assets/76876702/7abd69fd-d00e-4415-9672-2d71591e01a0

type and value change after refresh. See also that the date picker disappears on refresh because of the type change to Object, and now to edit the date the user edits a JSON string instead. Refreshing makes things consistent in this case

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51865

:watch: Updated May 16, 2024, 20:52 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

mtrezza commented 1 month ago

I tried out the latest commit.

iMac7 commented 1 month ago
  • Date: i think there is a bigger issue with date. The param type changes from date to object when the value is displayed as { type:...}. So this is not an edge case but looks more like a bug in this PR.

Not a bug in PR, I'm able to reproduce on alpha as well :thinking:

mtrezza commented 1 month ago

Oh interesting, maybe that is why this PR is having issues with the Date param.

mtrezza commented 1 month ago

I've opened https://github.com/parse-community/parse-dashboard/issues/2566 for the date issue. We can ignore that here for now.

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51874

:watch: Updated May 17, 2024, 00:03 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

mtrezza commented 1 month ago

1) Could you display the timestamp in the same format as dates are displayed in the data browser? I'm not sure if that is depending on the locale of the browser, or whether the format is fixed; you may want to look at the logic in the data browser:

image

It's a more compact format and includes the time zone, which is important.

2) There is still an issue with the default value and previous values. Here's an example:

- a) Create string param with value "a", click "create" to close dialog.
- b) Click on param to open dialog --> there should already be a "previous value" entry, which is the timestamp of when "a" was stored. That "previous value" should be preselected when the dialog is opened. It is actually not a "previous value" at this point but the "current value". If we don't do that then it can get confusing because once a previous value is selected, there is no way of knowing what the current value is. Of course, if a user selects the timestamp of the current value and clicks "save", then no save operation should occur, because the value hasn't changed.

3) Change the field name from "Previous value" to "Change history" and the description to "Select a timestamp in the change history to preview the value in the 'Value' field before saving."

4) The "change history" is currently only for values, not for whether the master key is required. It would be nice to include that in the history, to make this a more complete "rollback" feature. Not a requirement for this PR, we can merge without. It may however be difficult to add that later on, because after upgrading, a user will have a mixed history in the local browser store, some without and some with "master key required" flag. If the user then switches between these history entries, the toggle button will sometimes update, sometimes stay as is because there is no historic value, which can lead to mistakes.

iMac7 commented 1 month ago

4. The "change history" is currently only for values, not for whether the master key is required. It would be nice to include that in the history, to make this a more complete "rollback" feature. Not a requirement for this PR, we can merge without. It may however be difficult to add that later on, because after upgrading, a user will have a mixed history in the local browser store, some without and some with "master key required" flag. If the user then switches between these history entries, the toggle button will sometimes update, sometimes stay as is because there is no historic value, which can lead to mistakes.

If it's not a requirement, I would prefer to add this in future because-

mtrezza commented 1 month ago

It's not a requirement, I agree. Changing the master key toggle is also such a critical change that we need to think more about how to prevent a user from changing it without knowing (confirmation dialog, etc).

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51959

:watch: Updated May 18, 2024, 10:31 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

iMac7 commented 1 month ago

It's not a requirement, I agree. Changing the master key toggle is also such a critical change that we need to think more about how to prevent a user from changing it without knowing (confirmation dialog, etc).

Possibly a confirmation modal that only appears when the master key toggle changes. The handler can be set on the save button

mtrezza commented 1 month ago

Yep. We'll leave that idea for future readers 🙂

I've reviewed and it looks great now. Just GeoPoint and File I wasn't able to test. Either it doesn't work on mobile to create these params, or they are broken.

iMac7 commented 1 month ago

Just GeoPoint and File I wasn't able to test. Either it doesn't work on mobile to create these params, or they are broken.

Geopoint is broken on alpha, it produces a console error when trying to save something. If a geopoint is saved as an object, it should work immediately that is fixed.

I've disabled config history for files because I don't think it would be as useful or work right there.

mtrezza commented 1 month ago

GeoPoint seems broken, but could you implement the history logic for it anyway?

The history for file can make sense, because a file is not deleted by default, just because its reference in the DB has changed. That means the file still exists and if you just set the old path, the reference is intact. If a developer added logic to delete the previous file when the file reference is removed, that is up to them to consider it, and in that case the historic references would be broken.

Just these 2 things above, then I believe we are good to merge this.

iMac7 commented 1 month ago

GeoPoint seems broken, but could you implement the history logic for it anyway?

History works for any object type currently, it should work for geopoint as well.

mtrezza commented 1 month ago

Got it, then if you could add just the file history. Thanks for your patience in developing this feature, I believe it turned out really great.

iMac7 commented 1 month ago

Currently I just save an entry with name and url, but the problem comes when applying and saving the file value I selected from the dropdown. Would it work to save a file without all its fields present? That would involve modifying the save function to accept just name and url as arguments for files . A possible solution is to add all the metadata (should I do that?), but the file itself would not be there before clicking save, so if there is a check anywhere for that, it needs to be handled. An alternative would be to fetch the file right before clicking save, but that would add the file to the db again.

I can't think of many options to do this with as minimal modification as possible/without interacting with other entities like the db, or overwhelming the user's browser storage with files. With so many file types available, as a user, I would have to name my files well to actually remember what was in any, which would be solved by a preview. With the many different file types accepted, that can take some time to do.

mtrezza commented 1 month ago

It should be possible to re-construct the Parse.File object just from the meta data (that you'd store in the browser storage), without any actual file data being present, and then saving that. File data upload and file reference storing are 2 separate steps. At least if the file is set in a field of Parse.Object, not sure how the Cloud Config logic works, but I assume it should work similar. What you are saving is not the file (i.e. transmitting the file data to the server to create a Parse.File object with valid name reference) but the Cloud Config param (i.e. transmitting the re-constructed Parse.File to the server with a name reference that we assume is valid because the file has been stored before).

In the DB, a Cloud Config param of type file looks like this:

{
  __type: "File",
  name: "text.txt",
  url "http://172.168.0.1:1337/parse/files/appId/..."
}

If you save name in the value history, then you just manually re-create this JS object and convert it to a Parse.File instance via Parse.File.fromJSON(...). Then you save this to the Cloud Config param.

It's strange that the url is stored in the Cloud Config param as well, because that URL is dynamic and can change. Maybe url is ignored anyway and you don't need to store it, only the name may be sufficient.

See also https://github.com/parse-community/Parse-SDK-JS/issues/1456.

mtrezza commented 1 month ago

Did it work?

uffizzi-cloud[bot] commented 1 month ago

Uffizzi Ephemeral Environment deployment-51967

:watch: Updated May 18, 2024, 21:35 UTC

:cloud: https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2554

:page_facing_up: View Application Logs etc.

What is Uffizzi? Learn more

iMac7 commented 1 month ago

Did it work?

Yes, using the name and url to make a parsefile worked. I realized I was thinking about the native File() constructor initially

mtrezza commented 1 month ago

How many versions are stored in the history? I thought you mentioned 10, but I just made 13 entries and they keep being stored.

iMac7 commented 1 month ago

How many versions are stored in the history? I thought you mentioned 10, but I just made 13 entries and they keep being stored.

I had placed that limit, removed now

mtrezza commented 1 month ago

Why removed, do we need one?

iMac7 commented 1 month ago
  • How many previous values are stored, is there a limit in counts or are there any storage size limitations? Should the count of previous values be configurable as a dashboard option?

Not sure so I used an opinionated value of 10 history entries per parameter to be very light on the user's storage. I don't think we need that much data considering this feature is mostly about tracking the most recent changes 🤔

I set it here, eventually I removed it because you suggested along the thread that parse-server has no limit

mtrezza commented 1 month ago

These are 2 different questions:

Unless we look at storage size, it will be difficult to manage, because for a small config, many value versions will require little storage, while for a large config few versions require large storage. But looking at storage gives an arbitrary number of versions, which is difficult to manage for the user. Maybe the best is to implement a default of 100 versions, but make it configurable, so developers can adapt it to their use case. If you don't want to implement the configurable option now (it's quite easy), let's just leave it at 100.

If we don't implement any limits, then we may create a problem for users, because if the storage size becomes too large, the user would have to clear the browser cache for the website, loosing all other cached options as well.

iMac7 commented 1 month ago

These are 2 different questions:

  • a) Is there a limit on the server side? I'm not aware of any Cloud Config params limits, but there is a limit imposed by database constraints:

    All config params are stored in a single document. The max document size is 16MB. 16MB x 10 = 160MB max storage needs.

    That gives an upper limit for the max total size of Cloud Config params, regardless of how many params there are. It could be a single Cloud Config param that has ~16MB and that would reach the limit of how much data can be stored for Cloud Config params. Note that for params of type "File" this has nothing to do with the file size itself, as only the reference is stored.

  • b) Is there a limit on the browser side? That's why it could make sense to implement a limit, either on max. number of timestamps, or a storage size limit. I don't know how a browser manages website storage, whether there are any limits and what happens if limits are reached, for example does the browser slow down, show a warning? In theory, if a param of type "String" is ~16MB long, and there are 10 versions stored, it amounts to 160MB of storage needs. That's a theoretical case for sure, but I'm not sure how often the website storage is typically deleted (months, years?).

The only limit is the one I had set before, no other. I had just limited it to 10 items earlier, 100 looks better. 10 was strictly an opinionated value from me because I thought some sort of limit is needed.

Maybe the best is to implement a default of 100 versions, but make it configurable, so developers can adapt it to their use case

where in the dashboard should it be configured from?

mtrezza commented 1 month ago

You could just add an option apps.cloudConfigHistoryLimit to the dashboard config file. It would be on the app level, i.e. set for each app separately. Default value would be 100. See docs.

Btw, does the history work with multiple apps? For example app A has Cloud Config param x, and app B has the same. These history for both apps must be managed separately and not influence each other.