integrations: Assign default logo to integrations #23712

nagyem · 2022-11-30T18:59:26Z

If an integration does not have a logo, then a generic bot logo will be assigned. This allows the use of logos in documentation. Automated tests verify the creation of a generic logo when one is not given. My initial implementation made a copy of static/images/integrations/logos/generic.png to static/images/integrations/logos/{name}.png. I decided to instead just set self.logo_path=static/images/integrations/logos/generic.png without making a copy because the django staticfiles manifest was not updated when the file got created, causing a ValueError on line 130 in integrations.py return staticfiles_storage.url(self.logo_path)

The UI for the attribution is not particularly nice to look at. I think there needs to be more padding between the text and the image. Thoughts?

Fixes: #22582

Screenshots and screen captures:
Attribution:

Default logo used for "Generic Bot" in documentation:

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

alya · 2022-12-01T08:48:09Z

@nagyem Thanks for the PR! Please add a screenshot of an integration using the logo to the PR description, fix the failing tests, and post a comment when the PR is ready for review.

nagyem · 2022-12-04T17:49:06Z

Ready for review!

alya · 2022-12-07T06:44:38Z

templates/corporate/attribution.html

@@ -23,6 +23,11 @@ <h1>Website attributions</h1>
                    <img alt="" src="/static/images/landing-page/companies/software-engineer.svg" />
                    <p>"<a href="https://iconscout.com/illustration/software-engineer-2043023">Software engineer Illustration</a>" By <a href="https://iconscout.com/contributors/delesign/illustrations">Delesign Graphic</a> is licensed under <a href="https://creativecommons.org/licenses/by/4.0/">CC BY 4.0</a>.</p>
                </li>
+                <li>
+                    <b>Default Integration Logo</b>


Let's use sentence case here, as we generally do at Zulip, and match the formatting above of the other attribution label. I.e.:

Default integration logo:

alya · 2022-12-07T06:45:12Z

templates/corporate/attribution.html

+                    <b>Default Integration Logo</b>
+                    <img alt="" src="/static/images/integrations/logos/generic.png" />
+                    <p>"<a href="https://www.flaticon.com/free-icons/technology">Technology icons"</a> created by Freepik - Flaticon.</p>
+                </li>


Something seems off with quotation marks here.

Yeah, @nagyem can you add a commit at the top of this PR correcting the quotation marks used here. We should be using <p>"<a href='https://iconscout.com/illustration/software-engineer-2043023'>Software engineer Illustration</a>"

alya · 2022-12-07T06:46:11Z

Thanks! I left a couple of small comments. @amanagr , perhaps you could take a look as well?

I have not pulled down the PR for manual testing.

nagyem · 2022-12-09T17:40:53Z

@alya Reviewed your comments and made updates to the text to match the format of the other attribution.

Could you help me understand the build fail? I'm not familiar with Puppet.

alya · 2022-12-09T18:18:48Z

Could you help me understand the build fail? I'm not familiar with Puppet.

Please post a question in the #development help stream in the Zulip development community. Be sure to follow https://zulip.readthedocs.io/en/latest/contributing/asking-great-questions.html. Thanks!

amanagr · 2022-12-12T05:37:59Z

@nagyem can you provide steps for testing this locally?

nagyem · 2022-12-12T16:57:33Z

@amanagr

To run the backend tests:
Run ./tools/test-backend zerver/tests/test_integrations.py in the Zulip dev environment.

To manually test the default logo:
Add WebhookIntegration("alertmanager", ["misc"], display_name="New Integration"), to the WEBHOOK_INTEGRATIONS variable on line 326 in zerver/lib/integrations.py

(I used the name "alertmanager" because using a random name would cause an import error. "alertmanager" has a logo defined, but the logo path does not follow the default path generated/bots/{name}/logo.png, so without defining a logo, the generic bot will be used.)

Navigate to http://localhost:9991/integrations/ to see the generic logo being used.

amanagr

@nagyem thanks for the help with testing. LGTM other than the comment I made above. I would have liked for us to use svg format for the generic logo but I would leave it for TIm to comment on.

amanagr · 2022-12-14T16:38:07Z

templates/corporate/attribution.html

+                    <b>Default Integration Logo</b>
+                    <img alt="" src="/static/images/integrations/logos/generic.png" />
+                    <p>"<a href="https://www.flaticon.com/free-icons/technology">Technology icons"</a> created by Freepik - Flaticon.</p>
+                </li>


Yeah, @nagyem can you add a commit at the top of this PR correcting the quotation marks used here. We should be using <p>"<a href='https://iconscout.com/illustration/software-engineer-2043023'>Software engineer Illustration</a>"

alya · 2022-12-30T22:07:23Z

@nagyem Will you have a chance to address @amanagr 's feedback, or should someone else take over finishing the PR? Thanks!

nagyem · 2023-01-02T10:32:29Z

@alya I addressed @amanagr ’s comments in my last commit

alya · 2023-01-02T18:50:18Z

@alya I addressed @amanagr ’s comments in my last commit

Got it. For future reference, you need to post a comment when doing so, as otherwise reviewers don't know that it's time to take another look at the PR.

amanagr · 2023-01-05T10:22:27Z

@nagyem LGTM, can you move the last commit to be the first and improve its commit message based on https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-messages. Thanks!

nagyem · 2023-01-05T14:22:04Z

@amanagr @alya Done! Let me know if there's anything else I need to do.

alya · 2023-01-05T18:32:57Z

For some reason the logo shows up twice on the /attribution page.

Let's also add a line break after Default integration logo:, so that the logo isn't squashed up against the heading.

alya · 2023-01-05T18:33:42Z

The integrations page looks good to me in manual testing!

If an integration does not have a logo, then a generic bot logo will be assigned. This allows the use of logos in documentation. Automated tests verifythe creation of a generic logo when one is not given.

nagyem · 2023-01-05T19:41:41Z

@alya I pushed the change and rebased to keep the commit history clean. I added two breaks because one was not sufficient.

alya · 2023-01-05T19:57:22Z

Thanks! Please update the /attributions screenshot in the PR description with the latest version.

nagyem · 2023-01-05T20:01:35Z

@alya Done!

alya · 2023-01-05T20:14:10Z

@timabbott This PR has been reviewed by @amanagr and me -- please take a look!

zulipbot · 2023-02-14T22:27:29Z

Heads up @nagyem, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: L label Nov 30, 2022

nagyem force-pushed the main branch from 1bc57c7 to b45f5f2 Compare November 30, 2022 23:56

nagyem force-pushed the main branch 3 times, most recently from 2742c2d to 768a44c Compare December 3, 2022 20:04

zulipbot added size: M and removed size: L labels Dec 3, 2022

nagyem force-pushed the main branch 3 times, most recently from 198e6ce to 56e233e Compare December 4, 2022 17:23

alya reviewed Dec 7, 2022

View reviewed changes

nagyem force-pushed the main branch from d44436b to ef49d62 Compare December 10, 2022 23:03

amanagr reviewed Dec 14, 2022

View reviewed changes

zulipbot added size: L and removed size: M labels Dec 15, 2022

integrations: Fix quotation marks for attributions.

06b580a

nagyem force-pushed the main branch from ab66c02 to cb7cd2f Compare January 5, 2023 13:46

integrations: Assign default logo to integrations.

f6098d8

If an integration does not have a logo, then a generic bot logo will be assigned. This allows the use of logos in documentation. Automated tests verifythe creation of a generic logo when one is not given.

nagyem force-pushed the main branch from cb7cd2f to f6098d8 Compare January 5, 2023 19:39

alya added the integration review Added by maintainers when a PR may be ready for integration. label Jan 5, 2023

zulipbot added the has conflicts label Feb 14, 2023

timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrations: Assign default logo to integrations #23712

integrations: Assign default logo to integrations #23712

nagyem commented Nov 30, 2022 •

edited

alya commented Dec 1, 2022 •

edited

nagyem commented Dec 4, 2022

alya Dec 7, 2022

alya Dec 7, 2022

amanagr Dec 14, 2022

alya commented Dec 7, 2022

nagyem commented Dec 9, 2022

alya commented Dec 9, 2022

amanagr commented Dec 12, 2022

nagyem commented Dec 12, 2022 •

edited

amanagr left a comment •

edited

amanagr Dec 14, 2022

alya commented Dec 30, 2022

nagyem commented Jan 2, 2023

alya commented Jan 2, 2023

amanagr commented Jan 5, 2023

nagyem commented Jan 5, 2023

alya commented Jan 5, 2023

alya commented Jan 5, 2023

nagyem commented Jan 5, 2023

alya commented Jan 5, 2023

nagyem commented Jan 5, 2023

alya commented Jan 5, 2023

zulipbot commented Feb 14, 2023

integrations: Assign default logo to integrations #23712

Are you sure you want to change the base?

integrations: Assign default logo to integrations #23712

Conversation

nagyem commented Nov 30, 2022 • edited

alya commented Dec 1, 2022 • edited

nagyem commented Dec 4, 2022

alya Dec 7, 2022

Choose a reason for hiding this comment

alya Dec 7, 2022

Choose a reason for hiding this comment

amanagr Dec 14, 2022

Choose a reason for hiding this comment

alya commented Dec 7, 2022

nagyem commented Dec 9, 2022

alya commented Dec 9, 2022

amanagr commented Dec 12, 2022

nagyem commented Dec 12, 2022 • edited

amanagr left a comment • edited

Choose a reason for hiding this comment

amanagr Dec 14, 2022

Choose a reason for hiding this comment

alya commented Dec 30, 2022

nagyem commented Jan 2, 2023

alya commented Jan 2, 2023

amanagr commented Jan 5, 2023

nagyem commented Jan 5, 2023

alya commented Jan 5, 2023

alya commented Jan 5, 2023

nagyem commented Jan 5, 2023

alya commented Jan 5, 2023

nagyem commented Jan 5, 2023

alya commented Jan 5, 2023

zulipbot commented Feb 14, 2023

nagyem commented Nov 30, 2022 •

edited

alya commented Dec 1, 2022 •

edited

nagyem commented Dec 12, 2022 •

edited

amanagr left a comment •

edited