Add serverside persisted user preference trashed form list collapsedness #1018

matthew-white · 2024-08-09T02:27:34Z

This PR starts implementing the Frontend portion of getodk/central#689. I'm creating a draft PR so that we can discuss in GitHub.

matthew-white

Looks like good progress!

This might be on your radar already, but one thing I'm noticing is that currently, the preference doesn't vary by project. The description from @lognaturel says:

[T]he collapse state should be persistent on a per-project basis. For example, if I collapse the trash in projectA but not in projectB, I should always see a collapsed trash in projectA and always see an expanded trash in projectB until I toggle the show/hide state.

matthew-white · 2024-08-09T02:39:36Z

src/request-data/resources.js

@@ -77,6 +78,26 @@ export default ({ i18n }, createResource) => {
    transformResponse: ({ data }) => shallowReactive(data)
  }));

+  createResource('userPreferences', (self) => ({


I think the key thing here will be to make userPreferences reactive. Some requestData resources don't need to be reactive at all, and others only need to be shallow-reactive. We want to avoid the overhead of reactivity where possible, so by default, a resource is not reactive: reactivity is opt-in. You can use transformResponse() to make a resource reactive.

By example, above, I don't think session is ever used in a reactive context, so it doesn't need to be reactive (I also don't think session is ever patched, only cleared). On the other hand, data about currentUser can be changed and is shown in a reactive context, so currentUser does need to be reactive.

Got it, see 6a864fd

matthew-white · 2024-08-09T02:44:02Z

src/components/form/trash-list.vue

+  },
+  data() {
+    return {
+      isFormTrashCollapsed: this.userPreferences.data?.isFormTrashCollapsed


Once userPreferences is reactive, you shouldn't need a data property, which is a form of state. You can just access userPreferences.isFormTrashCollapsed directly in the template. Or you could define a computed property:

computed: { isFormTrashCollapsed() { return userPreferences.isFormTrashCollapsed; }, }

By the way, userPreferences.isFormTrashCollapsed is the same as userPreferences.data.isFormTrashCollapsed (a little bit of proxy magic).

Got it, see 6a864fd

matthew-white · 2024-08-09T02:52:07Z

src/request-data/resources.js

+        self.patch(Object.fromEntries(new Map([[k, v]])));
+        const headers = { 'Content-Type': 'application/json' };
+        self.request({
+          method: 'POST',
+          url: apiPaths.userPreferences(),
+          headers,
+          data: self.data,
+          patch: noop,
+        });


I like what you're doing here with patching userPreferences right away, then firing off the request, rather than waiting for the request to complete before patching. I can't think of another example of that in the frontend, so I don't think there's an established idiom for you to draw from. I think patch: noop is a nice solution. If we implement #675 at some point, that might allow patterns like this to be simpler.

I also opted for ignoring network errors here. So if there is a transient network error, the settings will not be saved serverside, only clientside - yet they will be saved serverside if the user pokes some other setting while the network has come back. We could deem that "good enough", but if someone insists on it I can try something elaborate elaborate with retries ;-)

When this happens, currently the user will see a toast "Something went wrong: there was no response to your request". I should probably silence that as there's really nothing (well, nothing reasonable right here and now) that the software or the user can do about it. What do you think? Silence the toast?

matthew-white · 2024-08-09T02:54:28Z

src/request-data/resources.js

+      // Thus superfluous requests are not 100% guaranteed to be filtered out.
+      const haschanged = JSON.stringify(self.data[k]) !== JSON.stringify(v);
+      if (haschanged) {
+        self.patch(Object.fromEntries(new Map([[k, v]])));


patch() is useful if you're changing multiple properties at once, but if you're just changing one property, you can set it directly:

self[k] = v;

matthew-white · 2024-08-09T02:59:26Z

src/components/form/trash-list.vue

@@ -63,6 +73,7 @@ export default {
  },
  created() {
    this.fetchDeletedForms(false);
+    this.fetchUserPreferences();


One question to consider is what should happen while this request is in progress. We know that some users will have slow connections. What should be rendered during the request, if anything?

With 6a864fd, if the request is still in progress (!userPreferences.dataExists), the default is to show the expanded state. If it later turns out that the lazyloaded prefs say otherwise, this may cause a UI jerk. So there's no wait, but there is a potential UI jerk, which can indeed be noticeable depending on network properties.

But by avoiding fetching the prefs incessantly (2c3672a) this problem is greatly reduced; the prefs really only need to be fetched once (when first needed), concurrent modification (from another session by the same user) of the prefs can be ignored.

Of course such concurrent sessions still end up fighting eachother's prefs (whereby the last write wins), loading the settings just once hides that such a fight is taking place. It is only when the user reloads their app (or restores a tab, reboots their computer, …), that they may then suddenly see their UI switched to the prefs of the other session (probably without understanding why).

As we've discussed, from "Settings Tied to User" it follows that a user can find themselves fighting two sessions against eachother (perhaps one session on a small screen, and another on a wider screen, different settings may be desired, which is not unifiable with "Settings Tied to User"!).

A strategy to make that better could be:

tie settings to sessions rather than to users

upon initialization of a new session, fork from the settings of the most recently active session of the user

I think we should focus on user-level settings for now and come back to session-specific settings if we see the need!

I believe it will be unusual for the same user to pick different values for the same setting in different sessions. I do think it's relatively likely that a user would change different settings in different sessions. That second situation feels like it would be handled well by a PATCH endpoint to only update one setting at a time. I haven't looked at any code yet so that may already be happening! I think writing all possible user settings at once to the server would make that second case too confusing. Interested to hear what @ktuite thinks too

I've touched upon these considerations in the PR description of #1021.

I've implemented incremental patching as suggested by @lognaturel here with 74a963b & getodk/central-backend@44e34f9 .

This mitigates the worst of potential user surprises as with this the server state becomes a mashup of preferences expressed across all app instantiations rather than an exact copy of some particular client state (of the client that happens to have done the last modifications).

Still, upon (for instance) an app reload, a user may be (surprised|pleased) to find some preferences expressed in a concurrent instantiation applied to the re-instantiated app in their tab. Moreover, they might not realize that they had expressed those preferences elsewhere and thus might not understand their emergence. Would documentation help, so that at least (whether the user agrees with the behaviour or not), it is clear that this is to be expected?

This behavior sounds great to me! I agree there could be some cases where the merging could be surprising in a negative way but I think pleased/satisfied/didn't notice anything because it behaved as expected will be most common reactions.

Yes, let's document this. How about filing an issue at https://github.com/getodk/docs/issues for all of this related work and keeping notes or a checklist of interesting things to include? These can be links to comments or pieces of code, whatever you think will best jog our memory once we write the docs. Thanks!

matthew-white · 2024-08-09T03:17:01Z

A number of tests are currently failing, but it's OK if you want to wait until later to look at those. Eventually, we'll want any new functionality to be tested, but it's totally OK for tests to come after you've implemented the functionality. Once you start looking at tests, you'll need to update test/util/http/data.js so that a default response is returned to ProjectOverview for userPreferences. I think that change alone will probably fix most test failures.

…vron-right have unequal width

brontolosone · 2024-09-04T16:14:59Z

Closing, superseded by #1021

(except I can't close this PR, @matthew-white or someone else might)

matthew-white commented Aug 9, 2024

View reviewed changes

brontolosone and others added 9 commits September 4, 2024 15:21

Add serverside persisted user preference trashed form list collapsedness

1347133

per-project form trash collapsedness

73d001d

only fetch prefs once, ignore concurrent modification

2613af6

comment elaboration

cad0d47

simplify

890f1ba

add userpreferences.fetchOnce() method

7f9f9cf

add project sort mode preference persistence

cbb9934

Fixate the formtrash expander width as icon-chevron-down and icon-che…

a8b4b0d

…vron-right have unequal width

don't alert the user to failed preference POST-ings

2609be4

brontolosone force-pushed the central_689-userpreferences branch from a968018 to 2609be4 Compare September 4, 2024 15:21

brontolosone mentioned this pull request Sep 4, 2024

Serverside-persisted user preferences #1021

Closed

lognaturel closed this Sep 6, 2024

This was referenced Sep 17, 2024

Add user preference persistence framework #1024

Draft

(Maybe) document potential suprises involving user preference persistence getodk/docs#1854

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add serverside persisted user preference trashed form list collapsedness #1018

Add serverside persisted user preference trashed form list collapsedness #1018

matthew-white commented Aug 9, 2024 •

edited

Loading

matthew-white left a comment

matthew-white Aug 9, 2024

brontolosone Sep 2, 2024

matthew-white Aug 9, 2024

brontolosone Sep 2, 2024

matthew-white Aug 9, 2024

brontolosone Sep 2, 2024 •

edited

Loading

matthew-white Aug 9, 2024

brontolosone Sep 2, 2024

matthew-white Aug 9, 2024

brontolosone Sep 2, 2024

lognaturel Sep 3, 2024 •

edited

Loading

brontolosone Sep 4, 2024

brontolosone Sep 6, 2024

lognaturel Sep 6, 2024

matthew-white commented Aug 9, 2024

brontolosone commented Sep 4, 2024 •

edited

Loading

Add serverside persisted user preference trashed form list collapsedness #1018

Add serverside persisted user preference trashed form list collapsedness #1018

Conversation

matthew-white commented Aug 9, 2024 • edited Loading

matthew-white left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brontolosone Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white commented Aug 9, 2024

brontolosone commented Sep 4, 2024 • edited Loading

matthew-white commented Aug 9, 2024 •

edited

Loading

brontolosone Sep 2, 2024 •

edited

Loading

lognaturel Sep 3, 2024 •

edited

Loading

brontolosone commented Sep 4, 2024 •

edited

Loading