[Citi OFN Voucher] Add VINE connected app #12886

rioug · 2024-10-02T06:07:49Z

⚠️ When working on this use the Clockify code: Citi Food Vouchers: #1A. #11922 Citi OFN Voucher - VINE Integration ⚠️

What? Why?

Closes [Citi Vouchers] Add VINE connected app #12855

Add VINE connected app, it allows an enterprise to provide a VINE API key and secret to be able to use VINE voucher for their enterprise.

A user is part of a team, so a successful call to /my-team endpoint mean the API key and secret are valid. It means the the API key needs at a minimum the "My Team Read" ability in VINE.

What should we test?

Turn on "connected_apps" feature toggle.
Make sure you have a valid API key and secret for vine staging server : https://vine-staging.openfoodnetwork.org.au/login

In the back office, logged as a super admin, enable Voucher Integration Engine (VINE) :

Go to "Configuration" tab, and click on connected app settings in the left hand side menu
Select "Voucher Integration Engine (Vine)"
Click "Update" to save the change

Still logged as a super admin:

Go to enterprise settings, for an enterprise you are not managing
Select "Connected Apps" on the left hand side menu
In the Voucher Integration Engine check :
- the VINE API Key, VINE secret inputs and Connect button are disabled
- It should say : "Only managers can connect apps"

As an enterprise manager:

Go to enterprise settings, for an enterprise you are managing
Select "Connected Apps" on the left hand side menu
Enter your API Key and secret and click "Connect" :
--> The API Key should get added to the enterprise, and you should see a "Disconnect" button
Click on "Disconnect" :
--> The API Key will be removed from the enterprise, and you should see an API Key and secret input and a "Connect" button
Enter a wrong API Key or secret and click "Connect" :
--> You should get an error message asking you to check if you entered your API Key and secret correctly
Enter only an API Key or a secret and click "Connect" :
--> You should get an error message saying API Key and secret can't be blank

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

⚠️ We need to provision production servers to add new configuration before the release, see dependencies below ⚠️

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

Dependencies

This PR requires an encryption key set to work :

ofn install PR : [Citi OFN Voucher] Add Activerecord encryption configuration and VINE api config ofn-install#942
Staging servers : https://github.com/openfoodfoundation/ofn-secrets/pull/102
Production servers : https://github.com/openfoodfoundation/ofn-secrets/pull/103

Documentation updates

The connection/disconnection logic is yet to be implemented

Generate a JWT token to be used to connect to the VINE api

It handles connection to the VINE API

Add logiv to connect and disconnect VINE API plus spec

dacook

Great work! This looks like a good fit for our "connected apps". I think there might be one line to clean up, other comments are pretty minor and don't need changes.

config/locales/en.yml

spec/services/vine_api_service_spec.rb

app/models/connected_apps/vine.rb

mkllnk

Really nice work, well done. Just a few little tweaks... ✨

config/locales/en.yml

spec/support/vcr_setup.rb

app/services/vine_api_service.rb

spec/services/vine_api_service_spec.rb

spec/models/connected_apps/vine_spec.rb

mkllnk · 2024-10-04T00:06:37Z

app/controllers/admin/connected_apps_controller.rb

+                   channel: SessionChannel.for_request(request))
+    end
+
+    def connect_vine


I think that we can leave it for now but it would be better to move app-specific logic into the apps and have the controller be agnostic of the app's needs.

To be honest I would prefer to have the connect logic to be separated from the model and having a service handle the connection bit before storing the result in the model.
But yeah, this should live in a service.

Good point, too. If we take the logic out of the models then we don't really need a polymorphic model. The beauty of the polymorphic model is that it loads the right code automatically. But reducing models to data storage is really handy as well. Then we just need service objects that are coupled to app types. I'm not sure which is better. Maybe it's just that the VINE app has some additional logic that should go into a service.

I notice that we're validating the presence of two values now, which we want to store in the model. ActiveRecord validations are made for that kind of thing (I guess it would have to be a custom validation to handle the values inside the data blob, but it still seems appropriate).
I'm not sure it's worth moving the logic right now though.

I feel like we might need other connected apps to see if a pattern emerges before we can refine the design. Maybe VINE is the odd one who knows. If I get a bit of time, I'll look at moving the VINE logic to a service.

mkllnk · 2024-10-04T00:07:26Z

app/controllers/admin/connected_apps_controller.rb

    def connected_app_params
-      params.permit(:type)
+      params.permit(:type, :vine_api_key)


We could ask the app to give us a list of allowed parameters.

spec/requests/admin/connected_apps_controller_spec.rb

The VINE Api require a secret and an API key to be used. The secret is used to sign the request. The secret is linked to the API key so we need to store it along side the key.

Added layer of security, we encrypt the API key and related secret. It requires setting up some encryption keys that can be generated wiht `bin/rails db:encryption:init`

mkllnk

Nice!

I queried Paul about the different handling of the secret but let's go ahead with this for now. We can use this interface either way.

mkllnk · 2024-10-08T00:13:19Z

app/controllers/admin/connected_apps_controller.rb

+                   channel: SessionChannel.for_request(request))
+    end
+
+    def connect_vine


Good point, too. If we take the logic out of the models then we don't really need a polymorphic model. The beauty of the polymorphic model is that it loads the right code automatically. But reducing models to data storage is really handy as well. Then we just need service objects that are coupled to app types. I'm not sure which is better. Maybe it's just that the VINE app has some additional logic that should go into a service.

rioug · 2024-10-08T01:05:27Z

.env

-# VINE API settings
-# VINE_API_URL="https://vine-staging.openfoodnetwork.org.au/api/v1"
-# VINE_API_SECRET="XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"


I was a bit too quick here, we stil need the VINE_API_URL 😅 , I'll fix that this afternoon

dacook

Nice, looking good, but as mentioned awaiting VINE_API_URL..

dacook · 2024-10-08T00:57:52Z

app/controllers/admin/connected_apps_controller.rb

+                   channel: SessionChannel.for_request(request))
+    end
+
+    def connect_vine


I notice that we're validating the presence of two values now, which we want to store in the model. ActiveRecord validations are made for that kind of thing (I guess it would have to be a custom validation to handle the values inside the data blob, but it still seems appropriate).
I'm not sure it's worth moving the logic right now though.

And add error handling if the variable is not set

spec/requests/admin/connected_apps_controller_spec.rb

The condition for checking the error now match a real scenario

RachL · 2024-10-14T13:32:00Z

@rioug all scenario are passing 🎉

I'm noticing that it is enough to be manager to have permission in the connected apps area. I wonder if we want to restrict to enterprise owners. Will ask in product circle.

In the meantime, merging :)

Add Vine connected app

0824430

The connection/disconnection logic is yet to be implemented

github-actions bot assigned rioug Oct 2, 2024

rioug added the user facing changes Thes pull requests affect the user experience label Oct 2, 2024

Add VineJwtService

f7708d6

Generate a JWT token to be used to connect to the VINE api

rioug force-pushed the 12855-VINE-connected-app branch from 6a6cc6e to 0c2dfe1 Compare October 2, 2024 06:17

Hide VINE token

1a30cf6

rioug force-pushed the 12855-VINE-connected-app branch from 0c2dfe1 to bde2f54 Compare October 2, 2024 06:19

rioug added feature toggled These pull requests' changes are invisible by default and are grouped in release notes and removed user facing changes Thes pull requests affect the user experience labels Oct 2, 2024

rioug added 3 commits October 2, 2024 16:44

Add VineApiService and specs

097c6de

It handles connection to the VINE API

Add logic for ConnectedApps::Vine#connect and disconnect

f980cb4

ConnectedApps controller, handle ConnectedApps::Vine

22428fc

Add logiv to connect and disconnect VINE API plus spec

rioug force-pushed the 12855-VINE-connected-app branch from bde2f54 to 22428fc Compare October 2, 2024 06:44

dacook requested changes Oct 3, 2024

View reviewed changes

mkllnk requested changes Oct 4, 2024

View reviewed changes

rioug added 4 commits October 7, 2024 11:23

Fix translation per review.

10c3c53

Per review, clean up code

224738e

Handle api secret

b14a1e7

The VINE Api require a secret and an API key to be used. The secret is used to sign the request. The secret is linked to the API key so we need to store it along side the key.

Add encryption for ConnectedApps::Vine#data

a3d8ae6

Added layer of security, we encrypt the API key and related secret. It requires setting up some encryption keys that can be generated wiht `bin/rails db:encryption:init`

rioug force-pushed the 12855-VINE-connected-app branch from 74234c2 to a3d8ae6 Compare October 7, 2024 04:10

rioug requested review from dacook and mkllnk October 7, 2024 04:38

rioug mentioned this pull request Oct 7, 2024

[Citi OFN Voucher] Add Activerecord encryption configuration and VINE api config openfoodfoundation/ofn-install#942

Merged

mkllnk approved these changes Oct 8, 2024

View reviewed changes

rioug commented Oct 8, 2024

View reviewed changes

dacook reviewed Oct 8, 2024

View reviewed changes

Re add VINE_API_URL env variable

df67b53

And add error handling if the variable is not set

rioug requested review from dacook and mkllnk October 8, 2024 04:06

dacook approved these changes Oct 8, 2024

View reviewed changes

spec/requests/admin/connected_apps_controller_spec.rb Outdated Show resolved Hide resolved

Fix spec checking if VINE api is set up

08308ba

The condition for checking the error now match a real scenario

mkllnk approved these changes Oct 8, 2024

View reviewed changes

RachL self-assigned this Oct 14, 2024

RachL added the pr-staged-au staging.openfoodnetwork.org.au label Oct 14, 2024

RachL merged commit f54552f into openfoodfoundation:master Oct 14, 2024
53 checks passed

RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Citi OFN Voucher] Add VINE connected app #12886

[Citi OFN Voucher] Add VINE connected app #12886

rioug commented Oct 2, 2024 •

edited by RachL

Loading

dacook left a comment

mkllnk left a comment

mkllnk Oct 4, 2024

rioug Oct 7, 2024

mkllnk Oct 8, 2024

dacook Oct 8, 2024

rioug Oct 8, 2024

mkllnk Oct 4, 2024

mkllnk left a comment

mkllnk Oct 8, 2024

rioug Oct 8, 2024

dacook left a comment

dacook Oct 8, 2024

RachL commented Oct 14, 2024

[Citi OFN Voucher] Add VINE connected app #12886

[Citi OFN Voucher] Add VINE connected app #12886

Conversation

rioug commented Oct 2, 2024 • edited by RachL Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

dacook left a comment

Choose a reason for hiding this comment

mkllnk 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

mkllnk left a 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

RachL commented Oct 14, 2024

rioug commented Oct 2, 2024 •

edited by RachL

Loading