-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
New settings screen to activate each connected app type #12721
New settings screen to activate each connected app type #12721
Conversation
6dfbc3f
to
31fdee0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great simple implementation. 👍
class ConnectedApp < ApplicationRecord | ||
scope :discover_regen, -> { where(type: "ConnectedApp") } | ||
scope :affiliate_sales_data, -> { where(type: "ConnectedApps::AffiliateSalesData") } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually define these as sub-classes of the migration. The table name still works and it's definitely another namespace to the original model.
Thankfully, good_migrations ensures that we don't load the actual model here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
I agree 👍 however @dacook I've tried 3 times on both FR and AU and I can't stage the pr :/ |
There were some good_migration error in bugsnag, ie: https://app.bugsnag.com/yaycode/openfoodnetwork-aus/errors/66b0ce0c5e9c074bc4c15904?event_id=66b0ce0c00f6a8c721040000&i=sk&m=nw |
@RachL ready to test on au_staging! If you are unable to test straight away, please remove stagin label so that Konrad is able to use it. |
I'm on it, I will try to do it in one go 👍 |
I disabled it before the PR was stage, and it did not re-enabled it. But I think this is normal. In any case we will be able to update it manually in production.
This is ok ✅ Thank you so much for handling this case @dacook I realize I've forgotten about it when writing the issue: I guess super admins should be able to do it, but it's a minor problem and can be dealt with in a different request (which I will open an issue for only if this becomes a problem). However:
I ended up with a white page and the app remains available in enterprise settings. @dacook Is it because it's enable by default on AU with the old config? In that case it won't be a problem in production and we can merge. Adding feedback label, but if it's ok feel free to merge 👍 |
The preference will be set from the admin interface in a new commit It would be nice if we had an array/list type for preferences. Probably not too hard to implement, but this will do.
It wasn't really necessary, but I'm going to need this list in a moment, so we might as well use it. Also it allows us to ensure the options are listed in a certain order. Also maybe it will help protect against corrupt preferences.
I considered adding a request spec, but figured it still doesnt' test the form, so better to use a full system spec.
Could have easily done this manually, but this makes the transition smoother. BTW I tested each case manually, didn't seem worth writing a spec.
84e4fc5
to
ffe3f12
Compare
Thanks @RachL , actually you uncovered a bug, oops! I have fixed this now, and added a success message when saving: But I have discovered something else that I might need to do for this PR, see comment on issue. |
If a checkbox is not selected, the browser does not submit it at all.
I'm not sure, but I assume that Config.set will raise an exception if it failed.
a291ef7
to
40c7794
Compare
Two new commits for review. |
Awesome, LGTM! Many thanks @dacook 🙏 Merging 💪 |
Funded feature code is #12543 [DFC] Anonymized orders endpoint
What? Why?
I forgot to make the new admin settings screen hidden behind the
connected_apps
feature toggle. But after this issue we will be able to remove the toggle anyway, so it's probably ok.Screenshot
What should we test?
As per issue. In addition:
connected_apps
feature toggle is enabledNote: Both connected app types are disabled by default, so the tab in Enterprise settings will not show until they are enabled.
Except for instances where connected apps are already in use:
Once-off migration
If there are enterprises with connected apps on the server already: then a once-off migration will enable them in Connected Apps Settings (to avoid them becoming unexpectedly hidden).
For example, on au_staging, discover_regen is in use, but affiliate_sales_data is not.
So it is expected that discover_regen will become enabled, but affiliate_sales_data will not.
Release notes
I guess "feature toggled". Although the settings screen can be accessed without the feature toggle (see above), it won't have any effect, so I don't think it's worth advertising.