Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from
May 19, 2024

Conversation

iMac7
Copy link
Contributor

@iMac7 iMac7 commented May 11, 2024

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

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)

Copy link

parse-github-assistant bot commented May 11, 2024

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@iMac7 iMac7 changed the title added config history feat: config history May 11, 2024
Copy link

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

@parse-github-assistant parse-github-assistant bot changed the title feat: config history feat: Config history May 11, 2024
Copy link

uffizzi-cloud bot commented May 11, 2024

Uffizzi Ephemeral Environment deployment-51593

⌚ Updated May 11, 2024, 01:46 UTC

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

📄 View Application Logs etc.

What is Uffizzi? Learn more

@mtrezza mtrezza changed the title feat: Config history feat: Add Parse Config history May 11, 2024
@mtrezza mtrezza changed the title feat: Add Parse Config history feat: Add Cloud Config history May 11, 2024
@mtrezza
Copy link
Member

mtrezza commented May 11, 2024

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
  • Previous values disappear: The first values were "a", "b" but they are not visible as there is no more space in the field.
  • Truncated values: The values "This is a long string that takes a" are indeed different values (they have different text), but they are indistinguishable as they are displayed.

Example 2:

image
  • Text overflow: If the text of the last items is long, it overflows the field and covers the buttons (cancel, ok) below, making it impossible to click the buttons. See the white text over the buttons.

Example 3:

image
  • Array values not properly displayed: The previous values in the example above were: [](empty array), [1], [1,2,3], [1,2,3,4,5,6,7,8,9]. In all cases the square brackets are missing.

Example 4:

image
  • Incorrect value entry: When clicking a previous value, it is incorrectly entered into the value field. For example, the previous value [1,2,3] is entered as 1,2,3, missing the brackets, which is an invalid array value.

Example 5:

image
  • Object values crash dashboard: Dashboard crashes when creating a config param of type "Object", entering the value {"a": true}, saving the param and then clicking on the param to open the edit dialog.

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

  • 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.
  • 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?

@iMac7
Copy link
Contributor Author

iMac7 commented May 11, 2024

  • 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 🤔

@mtrezza mtrezza closed this May 11, 2024
@mtrezza mtrezza reopened this May 11, 2024
Copy link

uffizzi-cloud bot commented May 11, 2024

Uffizzi Ephemeral Environment deployment-51601

⌚ Updated May 11, 2024, 21:58 UTC

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

📄 View Application Logs etc.

What is Uffizzi? Learn more

@mtrezza
Copy link
Member

mtrezza commented May 11, 2024

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
Copy link
Contributor Author

iMac7 commented May 12, 2024

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
Copy link
Member

mtrezza commented May 13, 2024

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:

  • Use a drop down list as UI element, not a scrollable list of buttons? There are several reasons why that is preferred.
  • Trying out the current implementation I'm not sure anymore that a foldable preview provides much value. In fact, clicking on an icon to open the preview is the same effort as clicking on the date to see the value in the value field. Only a tooltip seems to provide some value, as no click is needed. It's ok if the value is truncated, the purpose is a quick preview for simple values. For longer values (large objs or arrays) a user can inspect the value in the value field after selecting it. Not sure but adding an icon to a drop-down may require a customization of the UI element which may not be worth the effort.

@mtrezza mtrezza closed this May 13, 2024
@mtrezza mtrezza reopened this May 13, 2024
Copy link

uffizzi-cloud bot commented May 13, 2024

Uffizzi Ephemeral Environment deployment-51610

⌚ Updated May 13, 2024, 02:09 UTC

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

📄 View Application Logs etc.

What is Uffizzi? Learn more

@mtrezza mtrezza closed this May 14, 2024
@mtrezza mtrezza reopened this May 14, 2024
Copy link

uffizzi-cloud bot commented May 14, 2024

Uffizzi Ephemeral Environment deployment-51729

⌚ Updated May 14, 2024, 22:42 UTC

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

📄 View Application Logs etc.

What is Uffizzi? Learn more

@mtrezza
Copy link
Member

mtrezza commented May 14, 2024

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:

  • Either remove the tooltip feature altogether, or use the browser native tooltip instead, ensuring that it doesn't get in the way when using the drop-down. That means the tool tip should only appear when hovering over a separate "i" (info) icon on the right of the timestamp.
  • The value is incorrect; for example for string a it pastes the string with quotation marks "a". Could you please check for every type of config parameter (string, obj, array, ...) whether the value has the correct syntax after selecting a previous version?

@iMac7
Copy link
Contributor Author

iMac7 commented May 15, 2024

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.

@mtrezza
Copy link
Member

mtrezza commented May 18, 2024

Did it work?

@mtrezza mtrezza closed this May 18, 2024
@mtrezza mtrezza reopened this May 18, 2024
Copy link

uffizzi-cloud bot commented May 18, 2024

Uffizzi Ephemeral Environment deployment-51967

⌚ Updated May 18, 2024, 21:35 UTC

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

📄 View Application Logs etc.

What is Uffizzi? Learn more

@iMac7
Copy link
Contributor Author

iMac7 commented May 18, 2024

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
Copy link
Member

mtrezza commented May 19, 2024

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
Copy link
Contributor Author

iMac7 commented May 19, 2024

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
Copy link
Member

mtrezza commented May 19, 2024

Why removed, do we need one?

@iMac7
Copy link
Contributor Author

iMac7 commented May 19, 2024

  • 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
Copy link
Member

mtrezza commented May 19, 2024

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?).

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
Copy link
Contributor Author

iMac7 commented May 19, 2024

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
Copy link
Member

mtrezza commented May 19, 2024

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.

@iMac7
Copy link
Contributor Author

iMac7 commented May 19, 2024

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.

I hadn't thought of that, probably not. Let me check.
How do I make another parse app locally?
Between appid and server url, which is unique?

@iMac7
Copy link
Contributor Author

iMac7 commented May 19, 2024

{
  "apps": [
    {
      "serverURL": "http://localhost:1337/parse",
      "appId": "APPLICATION_ID",
      "masterKey": "MASTER_KEY",
      "appName": "",
      "iconName": "",
      "primaryBackgroundColor": "",
      "secondaryBackgroundColor": "",
      "cloudConfigHistoryLimit": 100
    }
  ],
  "iconsFolder": "icons"
}

That is an example of how the history limit is configured in dashboard config currently, lmk if that's right.

I have implemented unique history for every app by adding the application Id to the localstorage key of the app, if IDs are unique then the fix should hold.

One last review, hopefully :)

@mtrezza
Copy link
Member

mtrezza commented May 19, 2024

Yep that sounds perfect, great job, let's merge!

mtrezza
mtrezza previously approved these changes May 19, 2024
@mtrezza mtrezza changed the title feat: Add Cloud Config history feat: Add Cloud Config history to roll back to previous values May 19, 2024
@mtrezza
Copy link
Member

mtrezza commented May 19, 2024

Added README docs for new config option apps.cloudConfigHistoryLimit.

@mtrezza mtrezza changed the title feat: Add Cloud Config history to roll back to previous values feat: Add Cloud Config change history to roll back to previous values May 19, 2024
@mtrezza mtrezza merged commit a784129 into parse-community:alpha May 19, 2024
9 checks passed
parseplatformorg pushed a commit that referenced this pull request May 19, 2024
# [6.0.0-alpha.7](6.0.0-alpha.6...6.0.0-alpha.7) (2024-05-19)

### Features

* Add Cloud Config change history to roll back to previous values ([#2554](#2554)) ([a784129](a784129))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.0.0-alpha.7

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released-alpha Released as alpha version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add remote config history
3 participants