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: GUI actions without navigation #785

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

oskogstad
Copy link
Collaborator

Description

Adds writeAction to dialog GUI actions.
The property has a HTTP method (POST or DELETE), a boolean flag to indicate to the client whether or not a quick update to the dialog is expected , and an optional localization set for a prompt to show the user when triggering this action.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Documentation

  • Documentation is updated (either in docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)

@oskogstad oskogstad mentioned this pull request Jun 3, 2024
4 tasks
@oskogstad oskogstad force-pushed the feat/gui-actions-without-navigation branch from ffe83c5 to c040a9b Compare June 5, 2024 14:03
@oskogstad oskogstad changed the title feat: GUI actions without navigation [WIP] feat: GUI actions without navigation Jun 5, 2024
@oskogstad oskogstad force-pushed the feat/gui-actions-without-navigation branch 4 times, most recently from 853be56 to 0319579 Compare June 7, 2024 12:29
@oskogstad oskogstad marked this pull request as ready for review June 7, 2024 12:52
@oskogstad oskogstad requested a review from a team as a code owner June 7, 2024 12:52
@oskogstad
Copy link
Collaborator Author

oskogstad commented Jun 7, 2024

@elsand
We ended up adding prompt and httpVerb to GUI actions, and dropping the extra lookup type for Read/Write.
This can be inferred by front-end by looking at the httpVerb.
GET => READ, redirect
POST/DELETE => WRITE, no redirect

As for the IsDialogUpdateDelayed, @MagnusSandgren argued that this would be handled through subscriptions (for pushing updates)

But we would still have a problem if the UI is refreshed, front-end would then not know if the user has triggered the action or not. (This would not be helped by the IsDialogUpdateDelayed flag).
Could we use the same idea as for if dialog elements are read here?

Keep a log of triggered actions in dialogporten..
This way, front-end could show if

  • the current user has triggered the action
  • some other user has triggered the action, and current user cannot trigger again
  • some other user has triggered the action, and current user also has to trigger (partial signing, f.ex.)

@oskogstad oskogstad force-pushed the feat/gui-actions-without-navigation branch from bc29893 to 7d48644 Compare June 10, 2024 13:10
@elsand
Copy link
Member

elsand commented Jun 10, 2024

@elsand We ended up adding prompt and httpVerb to GUI actions, and dropping the extra lookup type for Read/Write. This can be inferred by front-end by looking at the httpVerb. GET => READ, redirect POST/DELETE => WRITE, no redirect

👍

As for the IsDialogUpdateDelayed, @MagnusSandgren argued that this would be handled through subscriptions (for pushing updates)

Yes, that was the idea. If that flag is false (or not set), it would indicate that the write operation would block until the dialog has been updated, so that the frontend could then immediately reload the dialog upon receiving a non-error response. If set, a GQL subscription would be set up (which presumably opens a websocket connection to Dialogporten, either directly from the client or via the BFF) which notifies the client as soon as the dialog is ready to be reloaded. Regardless of the flag, I would expect a spinner of some sort to be shown the user, blocking any interaction with any dialog actions until the dialog can be reloaded again.

But we would still have a problem if the UI is refreshed, front-end would then not know if the user has triggered the action or not. (This would not be helped by the IsDialogUpdateDelayed flag). Could we use the same idea as for if dialog elements are read here?

I'm not sure this UI-related state should be placed in Dialogporten. We would also have to use the somewhat unorthodox/lesser used 307 HTTP code in order to redirect a POST/DELETE with auth/CORS headers, which might cause compatibility issues in some browsers.

I'm thinking it's probably better to keep this state clientside in session storage. When the action is invoked, a key pending_update_<dialogid> with the value <initatedAt>,<updatedAt>,ws://pathtogqlchangesubscription could be stored. If the page is refreshed, that key can be checked for existance. If updatedAt for the dialog is (still) equal to the stored value, the spinner can be immediately re-shown, and the subscription connection re-established. (I'm not familiar with websocket/GQL subscription connection semantics, so maybe this isn't possble, in which case the frontend could instead just start polling the dialog for changes every few seconds.)

This of course won't stop any second users, or second browers for that matter, invoking any given action multiple times, but this is unlikely to be a problem in the real world - we require all actions to be idempotent anyway.

Clientside or not, we need to avoid ending up with the dialog being in a dead locked state when the request to the actual endpoint fails due to intermittent network issues or whatever. So there must be some sort of fixed threshold after which which we assume the action to have failed (eg. 10 seconds passed since initiatedAt) and just redisplay everything as is with an error message.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
9.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@MagnusSandgren
Copy link
Collaborator

MagnusSandgren commented Jun 11, 2024

If (IsDialogUpdateDelayed is) set, a GQL subscription would be set up (which presumably opens a websocket connection to Dialogporten, either directly from the client or via the BFF) which notifies the client as soon as the dialog is ready to be reloaded.

Should this subscription be set up based on one or several conditions, or actually set up every time dialog details are viewed? IMO there are several reasons to subscribe to a dialog when viewing it, every time. One of them being that you've pushed a button. Another being that someone else pushed a button, or the service owner has updated the dialog, or any other reason for the dialog to change.

I would argue that if dialog subscription was the default behavior the IsDialogUpdateDelayed, and any other such flag would be unnecessary. However, this is only possible if the technical side of the subscription solution can scale to our demands. If not, we would need to add these kinds of flags and subscribe conditionally.

I am thinking the following flow:

  1. User opens a dialog AND subscribes to that dialog
  2. User clicks a button, http request gets fired, button gets disabled and starts a spinner
  3. Http response is received:
    1. On success - button indicates success (animates a green check mark or something) and gets disabled (with a timeout as @elsand described)
    2. On error - button indicates error (shakes or something) and user can click again
  4. Some time after step 2 (async or sync) the subscription notifies the browser that a change has occurred, and refreshes (if the UI state is not dirty somehow, in which case the user must be presented with a question in the lines of "the dialog is updated, would you like to discard your changes or override someone else's changes". This is prob out of scope seeing as end user cannot sit on a dirty dialog?)

The beauty of this flow is that step 3 is rarely shown because of step 4.

@oskogstad
Copy link
Collaborator Author

@elsand @MagnusSandgren
Suggestion, since we don't have subscriptions yet,

  1. merge this as is and have a look at it when we start on subs
  2. start with subs on this PR

@elsand
Copy link
Member

elsand commented Jun 11, 2024

I would argue that if dialog subscription was the default behavior the IsDialogUpdateDelayed, and any other such flag would be unnecessary. However, this is only possible if the technical side of the subscription solution can scale to our demands. If not, we would need to add these kinds of flags and subscribe conditionally.

Whether or not a subscription is set up conditionally or not is ultimately up to the end-user system (ie. arbeidsflate). I don't think Dialogporten should (at least initially, lest we encounter scaling issues/abuse) actively refuse subscription requests for dialogs not containing non-GET GUI-actions. Having the flag lets other end-user systems that cannot utilize GQL subscriptions get a useful hint whether or not they can immediately reload the dialog upon request completion, or if they need to employ some other heuristic (ie. locally hide buttons or whatever).

Suggestion, since we don't have subscriptions yet, merge this as is and have a look at it when we start on subs

Agreed, the subscription feature is covered in #378 (which as of now describes a SignalR implementation, which I should probably update ...)

@oskogstad oskogstad merged commit f2d9136 into main Jun 12, 2024
13 of 14 checks passed
@oskogstad oskogstad deleted the feat/gui-actions-without-navigation branch June 12, 2024 07:52
@MagnusSandgren
Copy link
Collaborator

@elsand

Having the flag lets other end-user systems that cannot utilize GQL subscriptions get a useful hint whether or not they can immediately reload the dialog upon request completion, or if they need to employ some other heuristic (ie. locally hide buttons or whatever).

It's easy to fall into that trap thinking arbeidsflate is the only end user system. You're absolutely right.

oskogstad added a commit that referenced this pull request Jun 12, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.8.0](v1.7.1...v1.8.0)
(2024-06-12)


### Features

* Add support for external resource references in
authorizationAttributes
([#801](#801))
([1e674bd](1e674bd))
* Add user types
([#768](#768))
([b6fd439](b6fd439))
* Front channel embeds
([#792](#792))
([c3000bd](c3000bd))
* GUI actions without navigation
([#785](#785))
([f2d9136](f2d9136))
* Remove IsBackChannel concept from GUI Actions
([#819](#819))
([18101c1](18101c1))
* Rename IsDeleteAction to IsDeleteDialogAction
([#820](#820))
([18a1f6e](18a1f6e))
* **schema:** Rename MimeType to MediaType
([#813](#813))
([6490625](6490625))
* **schema:** undo setting performed by if not set
([#802](#802))
([c19f47a](c19f47a))


### Bug Fixes

* remove maskinporten aux from config
([#827](#827))
([2e4e81a](2e4e81a))
* **schema:** add package-lock file
([#804](#804))
([987dfa1](987dfa1))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants