Add new enterprise settings tab Connected Apps #11937

mkllnk · 2023-12-14T05:46:35Z

What? Why?

Closes Enterprise User controls data sharing #11718

Screencast.from.2023-12-15.16-31-13.webm

What should we test?

Testing requirement: the last step of testing needs the n8n workflow:

n8n workflow finished

Production test only

I realised that the n8n workflows are all tied to the Australian production server and therefore we can't test it in staging. We may change that but for now, we can just merge this. It's feature-toggled.

Testing steps:

Activate feature connected_apps for user, enterprise or instance.
Visit enterprise settings.
Select tab Connected apps
Click on Share data.
Maybe observe a loading message.
Then observe a notice to finish the setup.
Press on that button and see a form containing your enterprise details.

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

This depends on a certain n8n workflow. The webhook URL is hardcoded.

Documentation updates

I would have like to use a standard form to submit to the reflex but the whole enterprise settings tab is in a form already and HTML doesn't allow nested forms. While it does still work in browsers, it would have added much more HTML to set up a form with a hidden input field instead of just one additional data attribute. The whole page is rendered by the controller again but the reflex root attribute ensures that only parts of this tab are replaced. Otherwise unsaved data on other tabs could be replaced and the page actually becomes blank because AngularJS doesn't play well with the morph.

mkllnk · 2023-12-15T02:47:34Z

app/reflexes/admin/connected_app_reflex.rb

+# frozen_string_literal: true
+
+module Admin
+  class ConnectedAppReflex < ApplicationReflex


The StimulusReflex documentation mentions that singular and plural naming of reflexes is allowed and the choice depends on the circumstances. We use both in OFN. The plural seems to be useful when the reflex is listing or searching for collections, e.g. many orders. I decided to keep it simple and go with the singular. But over time, we may realise that any reflex could be expanded to deal with collections as well and then the plural would be better. To avoid renaming, we may introduce a convention but I don't see enough of a pattern yet.

mkllnk · 2023-12-15T03:28:52Z

app/views/admin/enterprises/form/_connected_apps.html.haml

@@ -1,11 +1,17 @@
-%div
+%div{ data: {reflex: {root: ".connected-app__head, .connected-app__connection"}} }


The reflex root will be replaced later.

mkllnk · 2023-12-15T03:31:01Z

spec/jobs/connect_app_job_spec.rb

+  it "stores connection data on the app" do
+    subject.perform_now
+
+    expect(app.data).to eq({ "link" => "https://example.net/edit" })


The URL will change on the live endpoint once it's implemented. We may not be able to do live recordings then because we don't want to add our test enterprise to the live database. Maybe we can have a test mode, let's see.

mkllnk · 2023-12-15T03:32:13Z

app/jobs/connect_app_job.rb

+
+    return unless channel
+
+    selector = "#edit_enterprise_#{enterprise.id} #connected_apps_panel div"


If you have multiple tabs open with other enterprises then you don't want to show this enterprise's information on another enterprise's page.

mkllnk · 2023-12-15T03:35:26Z

app/reflexes/admin/connected_app_reflex.rb

+      # Avoid race condition by sending before enqueuing job:
+      cable_ready.morph(selector:, html:).broadcast


The change on the page is almost instant now. Previously, it was calling the edit action of the enterprises controller and there was a noticeable lag. That action renders lots of data, including a select box with all enterprises of the platform and all kind of options for every tab of the enterprise settings. In contrast, the partial is very small.

dacook

Great implementation, well done!

It looks good too. The only thing is it would be good to use some CSS variables, which should help with the long-term maintenance of the stylesheet.

dacook · 2023-12-15T03:59:55Z

app/views/admin/enterprises/form/_connected_apps.html.haml

+      %h3= t ".title"
+      %p= t ".tagline"


I like this bracket-less format, I can't believe we weren't doing this before!
Now if only we could make lazy-lookup the default, and go back to symbols it would be beautiful 💎

We could add our own helper:

# I18n lazy translate def lt(key, *args) I18n.t(".#{key}", *args) end

What do you think? Saves one character, I suppose:

= lt :tagline

And it wouldn't be well known. Not worth it.

dacook · 2023-12-15T04:01:08Z

app/webpacker/css/admin/connected_apps.scss

+  margin-bottom: 1em;
+
+  h3 {
+    margin-bottom: 1em;


I would like to think we already have reasonable margins for h3 and p by default, but I guess not.

I would have thought so, too. But they must be overridden somewhere. Maybe it will be better in the new design?

dacook · 2023-12-15T04:05:31Z

spec/models/connected_app_spec.rb

+    subject.save!
+    subject.reload
+
+    expect(subject.data).to eq({ "link" => "https://example.net" })


Nice, I wonder if Rails has a way for it to be with_indifferent_access by default and therefore load the hash with symbol keys.

We would need to add a few things:
https://gist.github.com/RobinDaugherty/5e7bc2a6e2369a29c77c

Or we just override a method on the model:

def data data.try(:with_indifferent_access) end

I looked at this and we currently have only one single line of access to this data:

enterprise.connected_apps[0].data&.fetch("link", false)

And in this scenario it would only change "link" to :link which isn't making a real difference. I'll leave this.

dacook · 2023-12-15T04:17:54Z

app/webpacker/css/admin/connected_apps.scss

+  border-left: 4px solid #008397;
+  border-radius: 4px;


Ideally we'd use a variable for the colour. $color-3 is spree-blue in the old design, and the new teal in the new design.
Also, we can use a variable for border-radius. This helps ensure the left border is the same width as the radius.

Suggested change

border-left: 4px solid #008397;

border-radius: 4px;

border-left: $border-radius solid $color-3;

border-radius: $border-radius;

This will look a bit different on the old design, but I think should be ok.

There could also be a variable for box-shadow, but I have a PR in progress for that, so let's just leave it hardcoded here for now.

Ah, good to know. So $color-3 is available in the old and new design? I guess, once the new design is released, we can do a simple search/replace.

This is much better!

dacook · 2023-12-15T04:20:15Z

app/jobs/connect_app_job.rb

+      access_token: token,
+    }
+
+    response = WebhookDeliveryJob.perform_now(url, event, payload)


Yay, nice to see the WebhookDeliveryJob in use!

I just noticed this is a wrapper for WebhookDeliveryJob. Would it make sense to subclass it I wonder? I can't think it would make much difference..

In doubt, I tend to avoid inheritance. This job could be seen as a special webhook job which also captures the response. But it's also doing UX stuff and it may expand one day. Only part of it is the webhook functionality. So I think that composition is the better pattern here.

dacook

Oops that was meant to be a 'request changes' for the CSS variables.

dacook

Cool, I have to admit I accidentally missed the last two commits before.
But it all looks fine 👍

rioug

Nice one, I like your use of a reflex.

rioug · 2023-12-17T23:42:06Z

app/reflexes/admin/connected_app_reflex.rb

+
+module Admin
+  class ConnectedAppReflex < ApplicationReflex
+    def create


I know it's tested in a system spec, but I wonder if we should also have unit test for reflexes ?

Good question. I find unit tests good for testing individual methods with all the different code branches. But this method doesn't actually have any branching. And that means that the system spec is providing 100% code coverage. So I wouldn't bother at this point.

Actually, I overlooked one thing. The authorize! call is a code branch. We could test permissions in a unit test. Same for find and create! which could raise errors. But that's well-known behaviour we deal with all the time.

mkllnk added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Dec 14, 2023

mkllnk self-assigned this Dec 14, 2023

mkllnk force-pushed the dfc-app branch 2 times, most recently from a815014 to 702e93e Compare December 14, 2023 22:19

mkllnk added 9 commits December 15, 2023 11:22

Add new enterprise settings tab Connected Apps

af2b652

Connected apps can be enabled per user or enterprise

d4ce396

Add basic description of Discover Regen app

ca7b02d

Add ConnectedApp model

2346f03

Query webhook to connect app

12e5a0d

Show app connection state and update link

e1730f2

Broadcast connected app to all user tabs

1d7f96e

Respond fast by rendering only partial

35ca66c

mkllnk force-pushed the dfc-app branch from 702e93e to 35ca66c Compare December 15, 2023 01:35

mkllnk marked this pull request as ready for review December 15, 2023 01:39

mkllnk commented Dec 15, 2023

View reviewed changes

dacook approved these changes Dec 15, 2023

View reviewed changes

dacook requested changes Dec 15, 2023

View reviewed changes

Use common colours in old and new design

f758daa

mkllnk requested a review from dacook December 15, 2023 05:28

dacook approved these changes Dec 15, 2023

View reviewed changes

mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Dec 17, 2023

mkllnk added the pr-no-test label Dec 17, 2023

rioug approved these changes Dec 17, 2023

View reviewed changes

mkllnk merged commit b52b0c0 into openfoodfoundation:master Dec 18, 2023
56 checks passed

mkllnk deleted the dfc-app branch December 18, 2023 02:37

mkllnk removed the pr-staged-au staging.openfoodnetwork.org.au label Dec 18, 2023

mkllnk mentioned this pull request Dec 20, 2023

Enterprise user can remove Connected Apps #11954

Merged

4 tasks

mkllnk mentioned this pull request Dec 21, 2023

Send semantic id when connecting enterprise to app #11959

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new enterprise settings tab Connected Apps #11937

Add new enterprise settings tab Connected Apps #11937

mkllnk commented Dec 14, 2023 •

edited

mkllnk Dec 15, 2023

mkllnk Dec 15, 2023

mkllnk Dec 15, 2023

mkllnk Dec 15, 2023

mkllnk Dec 15, 2023

dacook left a comment

dacook Dec 15, 2023

mkllnk Dec 15, 2023

dacook Dec 15, 2023

mkllnk Dec 15, 2023

dacook Dec 15, 2023

mkllnk Dec 15, 2023

mkllnk Dec 15, 2023

dacook Dec 15, 2023

dacook Dec 15, 2023

mkllnk Dec 15, 2023

mkllnk Dec 15, 2023

dacook Dec 15, 2023

dacook Dec 15, 2023

mkllnk Dec 15, 2023

dacook left a comment

dacook left a comment

rioug left a comment

rioug Dec 17, 2023

mkllnk Dec 18, 2023

		@@ -1,11 +1,17 @@
		%div
		%div{ data: {reflex: {root: ".connected-app__head, .connected-app__connection"}} }


		return unless channel

		selector = "#edit_enterprise_#{enterprise.id} #connected_apps_panel div"

		# Avoid race condition by sending before enqueuing job:
		cable_ready.morph(selector:, html:).broadcast

Add new enterprise settings tab Connected Apps #11937

Add new enterprise settings tab Connected Apps #11937

Conversation

mkllnk commented Dec 14, 2023 • edited

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

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

dacook 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk commented Dec 14, 2023 •

edited