Add "Affiliate Sales Data" connected app option #12676

dacook · 2024-07-16T06:00:16Z

Funded feature code is #12543 [DFC] Anonymized orders endpoint

What? Why?

Closes As a enterprise user, I can access all connected apps under the same enterprise settings view #12550

Creates a second type of connected app, called "Affiliate Sales Data" in the backend (named after the work-in-progress affiliate_sales_data endpoint), and translated for specific use case.
I'm not 100% sure about the naming and how the scopes are set up. And it looks like the Discover Regen connected app should have its own subtype. But I didn't think it was worth trying to guess on my own how this might be expanded in the future, so just did the minimum necessary.

New connected app on enterprise screen (disabled by default) (it was to be enabled by default, but I misunderstood😞)
New method to access affiliate enterprises
Rake task to opt all enterprises in (this can be executed once on FR, then enterprises can choose to opt out as desired)

What should we test?

Ensure feature toggle connected_apps is enabled
Visit Enterprise edit screen and choose Connected apps tab
Observe both Discover Regen and INRAE sections
Discover Regen should work as before (note we will need to hide this for FR, this will happen in separate PR)
INRAE section requires manager permission. Go to users tab and add yourself if not already.
INRAE section is disabled by default.
INRAE can be enabled, and then disabled
Confirm the change doesn't affect other enterprises.

This option doesn't have any effect until #12544 is completed.

Rake task

This can tested by logging into the staging server and running something like:

bundle exec -e staging ofn:enterprises:activate_connected_app_type[affiliate_sales_data]

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

Not dependent on, but related to:

As a dedicated user, I can access a DFC API endpoint with fewer data and all enterprises #12544

To make way for a new type of connected app. If only we could use [relative partial paths](rails/rails#1143)

Then subtypes can override as needed.

Best viewed with whitespac ignored.

Because there's going to be a new section with the same button label

Using namespace subfolder to help organise it and show the inheritance. Hmm, instead of scopes, we could have different has_many relationships on the Enterprise. Maybe it should be in a concern. We can refactor later I guess.

Example usage: rake ofn:enterprises:activate_connected_app_type[affiliate_sales_data]

rioug

Nice work, easy to follow !

rioug · 2024-07-29T03:45:41Z

app/models/connected_app.rb

+  def connect(api_key:, channel:)
+    ConnectAppJob.perform_later(self, api_key, channel:)
+  end
+
+  def disconnect
+    WebhookDeliveryJob.perform_later(
+      data["destroy"],
+      "disconnect-app",
+      nil
+    )
+  end


~~Wouldn't that be better in a service ?~~
It made more sense now that I have seen the rest of the commits. I am still not a fan but right now I can't really think of another solution without bringing more complexity.

Yeah it feels wrong sending the websocket channel to the model. But with a service, I guess the controller has to know about the subtypes, and make the decision what to do with them, which doesn't seem any better.
I'm curious what Maikel thinks too.

I mean you could have a service that knows how to connect/disconnect each type of connected app, but that would add some indirection.
It could make sense if the connect/disconnect was a setting not stored with the model.

rioug · 2024-07-29T03:59:21Z

lib/tasks/enterprises.rake

+    desc "Activate connected app type for ALL enterprises"
+    task :activate_connected_app_type, [:type] => :environment do |_task, args|
+      Enterprise.find_each do |enterprise|
+        next if enterprise.connected_apps.public_send(args.type.underscore).exists?


I am wondering if we should whitelist the type param here ? Probably not necessary as it will blow up if you are trying to use a scope that doesn't exist.

Yeah I figured for a rake task that requires system access to execute, we don't need to add much protection.
Maybe I should have hardcoded the type (there's only one type after all!).

mkllnk

This is great! Nice pull request. I had one suggestion but it's not important enough to delay this pull request. Let's go ahead.

mkllnk · 2024-07-30T04:00:50Z

app/jobs/connect_app_job.rb

@@ -20,7 +20,7 @@ def perform(app, token, channel: nil)
    selector = "#connected-app-discover-regen.enterprise_#{enterprise.id}"
    html = ApplicationController.render(
      partial: "admin/enterprises/form/connected_apps/discover_regen",
-      locals: { enterprise:, connected_app: enterprise.connected_apps.first},
+      locals: { enterprise:, connected_app: enterprise.connected_apps.discover_regen.first },


I guess that the more generic alternative would be ConnectedApp.of(enterprise).first. We'll see with more usage what will be better.

mkllnk · 2024-07-30T04:34:02Z

app/models/connected_apps/affiliate_sales_data.rb

+    def connect(_opts)
+      update! data: true # not-nil value indicates it is ready
+    end


Could we set this value in the constructor so that we don't have to update the record again? Doesn't really matter though.

Good thinking

app/models/spree/user.rb

RachL · 2024-07-30T14:14:12Z

@dacook this all looks good to me. Thank you very much 🎉

Of course, I will need to re-test when #12544 is ready.

And I don't have ssh power to run the rake task, but again something we can test in a separate work, so merging!

github-actions bot assigned dacook Jul 16, 2024

dacook force-pushed the add-affiliate_sales_data-connected_app-12550 branch from 5a4de04 to c5c2f8f Compare July 17, 2024 01:50

dacook mentioned this pull request Jul 17, 2024

Add DFC anonymous orders endpoint #12573

Closed

4 tasks

dacook added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Jul 17, 2024

dacook changed the title ~~Add "Affiliate Sales Data" connected app~~ Add "Affiliate Sales Data" connected app option Jul 17, 2024

dacook added 10 commits July 25, 2024 17:05

Move discover regen to named partial

9a4051f

To make way for a new type of connected app. If only we could use [relative partial paths](rails/rails#1143)

Expect single connected_app record for discover regen

85d5e2e

AddTypeToConnectedApps

bbe22bb

add scope for discover_regen

9b37eac

Move connect logic to model

9d89b47

Then subtypes can override as needed.

Re-organise spec

5d0f55b

Best viewed with whitespac ignored.

Scope spec to section

27e53f9

Because there's going to be a new section with the same button label

Add AffiliateSalesData model

d3c5e23

Using namespace subfolder to help organise it and show the inheritance. Hmm, instead of scopes, we could have different has_many relationships on the Enterprise. Maybe it should be in a concern. We can refactor later I guess.

Add affiliate sales data to form

1742d28

Enable/disable affiliate sales data

da7bbcf

dacook force-pushed the add-affiliate_sales_data-connected_app-12550 branch 2 times, most recently from 37afae9 to a6d70d9 Compare July 25, 2024 11:04

dacook added 2 commits July 25, 2024 21:14

Add User#affiliate_enterprises

e9d7a0b

Add task to connect all enterprises

df81e8e

Example usage: rake ofn:enterprises:activate_connected_app_type[affiliate_sales_data]

dacook force-pushed the add-affiliate_sales_data-connected_app-12550 branch from a6d70d9 to df81e8e Compare July 25, 2024 11:14

dacook marked this pull request as ready for review July 25, 2024 11:14

rioug approved these changes Jul 29, 2024

View reviewed changes

mkllnk approved these changes Jul 30, 2024

View reviewed changes

Use scope to determine which enterprises are ready to be affiliated

c5fc621

dacook requested a review from mkllnk July 30, 2024 05:22

mkllnk approved these changes Jul 30, 2024

View reviewed changes

RachL added the pr-staged-fr staging.coopcircuits.fr label Jul 30, 2024

RachL self-assigned this Jul 30, 2024

RachL merged commit f51705c into openfoodfoundation:master Jul 30, 2024
52 checks passed

RachL removed the pr-staged-fr staging.coopcircuits.fr label Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Affiliate Sales Data" connected app option #12676

Add "Affiliate Sales Data" connected app option #12676

dacook commented Jul 16, 2024 •

edited

Loading

rioug left a comment

rioug Jul 29, 2024 •

edited

Loading

dacook Jul 29, 2024

rioug Jul 29, 2024

rioug Jul 29, 2024

dacook Jul 29, 2024

mkllnk left a comment

mkllnk Jul 30, 2024

mkllnk Jul 30, 2024

dacook Jul 30, 2024

RachL commented Jul 30, 2024

Add "Affiliate Sales Data" connected app option #12676

Add "Affiliate Sales Data" connected app option #12676

Conversation

dacook commented Jul 16, 2024 • edited Loading

What? Why?

What should we test?

Rake task

Release notes

Dependencies

rioug left a comment

Choose a reason for hiding this comment

rioug Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

dacook Jul 29, 2024

Choose a reason for hiding this comment

rioug Jul 29, 2024

Choose a reason for hiding this comment

rioug Jul 29, 2024

Choose a reason for hiding this comment

dacook Jul 29, 2024

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Jul 30, 2024

Choose a reason for hiding this comment

mkllnk Jul 30, 2024

Choose a reason for hiding this comment

dacook Jul 30, 2024

Choose a reason for hiding this comment

RachL commented Jul 30, 2024

dacook commented Jul 16, 2024 •

edited

Loading

rioug Jul 29, 2024 •

edited

Loading