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

[City OFN Voucher] Fix VINE connected app settings #13016

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rioug
Copy link
Collaborator

@rioug rioug commented Dec 3, 2024

What? Why?

Split the description and link to VINE in two translatable string. Description is now hidden when an enterprise is connected to VINE.

What should we test?

  • Go to /admin/enterprises/NAME/edit#/connected_apps_panel for an enterprise not connected to VINE yet
  • Check the link to VINE is correct
  • Check the sentence "To enable VINE for your enterprise, enter your API key and secret." appears
  • Add your API key
  • Check the sentence "To enable VINE for your enterprise, enter your API key and secret." does not appear anymore

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.

Description is now hidden when the enterprise is connected to VINE
@rioug rioug added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Dec 3, 2024
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that you're hiding the description once connected, because it describes how to connect. Maybe we should have a different string for each case, eg enabled_description_html and disabled_description_html (this could be helpful for other connected apps too).

Also this is related to #13011
On that PR, Konrad is saying that they would prefer to not have to edit HTML, so would need separate keys for the link text and href.

So... I'm thinking it might be time to manage these strings in the DB, and give instance managers an easier way to update them (with a rich text editor). Maybe we can fit that in with this funded feature? I'll mention that on the other PR.

@rioug
Copy link
Collaborator Author

rioug commented Dec 8, 2024

@RachL what do you think of David's suggestion ?

@RachL
Copy link
Contributor

RachL commented Dec 9, 2024

@rioug @dacook It's a nice idea, but I'm not 100% comfortable with the global spreadsheet just yet to confirm we have funding for this. I suggest for now we proceed with PRs handling translation and I will create something in the backlog and hopefully we can fund it.

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I think this is fine to go ahead, and is compatible with my idea in case we do that in the future.

rioug added a commit to drummer83/openfoodnetwork that referenced this pull request Dec 10, 2024
@RachL RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Dec 11, 2024
@RachL
Copy link
Contributor

RachL commented Dec 11, 2024

@rioug small fix needed: when not connected or disconnected the link is showed twice:

image

otherwise all good 💪 back to in dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature toggled These pull requests' changes are invisible by default and are grouped in release notes
Projects
Status: In Progress ⚙
Development

Successfully merging this pull request may close these issues.

[City OFN Voucher] Change link + remove a sentence on settings page
4 participants