🐞 Reset dataChanged when entering Edit mode for the Credentials section

[sgratch]

Reference: #789

Fixing a bug by resetting the dataChanged flag, whenever entering the edit mode for the Credentials section page.

Before

Screencast.from.2024-03-21.18-21-30.webm

After

Screencast.from.2024-03-21.18-23-53.webm

[yaacov]

i prefer that the reset of the dataChanged field will have it's own reducer, SET_DATA_CHANGE or RESET_DATA_CHAGE and it should be called explicitly after a successful patchSecretData

a. Calling the reducer explicitly after successful updating secret operation will make it more clear why we set this value to false, e.g. because we just set the secret value to match the state, so no state data change from the saved value.
b. Calling the reducer explicitly will allow us to leave the TOGGLE_EDIT action clean, unlike the SET_NEW_SECRET where it's easy to understand why we set the dataChanged flag , in the toggle edit, it's not clear why we change the value of dataChanged , the logic of why we set it to false is complex, and depend on the functionality of state.edit, e.g. if in some future we will change the usage of state.edit, we may break the dataChange unintentionally

[sgratch]

As far as I saw it when fixing, regardless to the patchSecretData logic, it should be always TRUE that when you call TOGGLE_EDIT action for switching from Non view mode to View mode, regardless to the previous state you switched from, the data state should be marked as unchanged.

On most usages, you enter the view mode by 2 ways:

1. Clicking cancel and losing your changes
2. Clicking update and updating your changes

If you are in a view mode and the data is marked as changed then you probably did something wrong and lost your changes without confirming it....

I get your point where you want the toggle edit to be as simple as possible just for toggling and put all logic in separate reducers but it is more buggy and I think it's the toggle edit responsibility.

[yaacov]

regardless to the patchSecretData logic, it should be always TRUE that when you call TOGGLE_EDIT

In the current suggestion we have an if else statment, so it's not always set to true.

but it is more buggy

My review suggest to remove an if else statement that is not needed and placed in the wrong place, I am missing you point

On most usages, you enter the view mode by 2 ways:

you are missing a 3rd option of failing to save changes to DB

My concerns with your current suggestion

const dataChanged = !state.edit ? false : state.dataChanged;
return { ...state, dataChanged, edit: !state.edit };

are:
a. it's in the wrong place, we should set the flag dataChanged meaning staged === saved after we did some action that changes staged or saved to be equal or changed, this places are when we change the staged secret or when we save the staged secret to the data base.

b. your suggestion requires an "if-else" ( in this case ? : operation) that can be avoided.

In the current code we set dataChanged to true after we update the staged data.
We do not reset the dataChanged to false after we save to the data base.

please update to set dataChanged after successful save

[sgratch]

I understand your points.

My suggested solution was to look at the theoretical view-mode state (implemented by state.edit === FALSE) and make sure that whenever we switch to that state, the dataChanged state is set to FALSE since there is no flow in which we enter view mode and secret data is in staged mode.

Handling that in the TOGGLE_EDIT action with if-else is just because we use that to toggle between 2 states: edit mode and view mode.

you are missing a 3rd option of failing to save changes to DB

In that case you can't leave the 'edit-mode' state (buttons are disabled in case of patching data erros) and if you enforce leaving it then you lose your changes.

---

Anyway, if you don't like it then I'm totally ok with fixing the way you suggested. Just need to keep in mind to call that new RESET_DATA_CHANGE action whenever we also change to view mode, or ask the user for confirming loosing the data if not.....

[yaacov]

See review comment

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud