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

Added the function get_gravatar_url with Unit Test #11800

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nandini584
Copy link
Contributor

This is an extension of PR #11077, where a new improvement of adding gravatar images was to be added, I have done the same and written two test cases, first for default size and second for custom_size, the function increases the original size to 2X and generated a safe_md5 hash for the email to constitute a URL,

Below is the screenshot of the passing test case.

Screenshot 2024-03-26 103751

Copy link

squash-labs bot commented Mar 26, 2024

Manage this branch in Squash

Test this branch here: https://nandini584improvement-to-grava-j1fcj.squash.io

@lb-
Copy link
Member

lb- commented Mar 26, 2024

@nandini584 thank you.

Could you please look at the failing test

======================================================================
FAIL: test_adapt (wagtail.admin.tests.ui.test_sidebar.TestAdaptMainMenuModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/wagtail/wagtail/wagtail/admin/tests/ui/test_sidebar.py", line 235, in test_adapt
    self.assertEqual(
AssertionError: {'_ty[168 chars]'', 'attrs': {}, 'url': '/pages/'}]}], [{'_typ[461 chars]0'}]} != {'_ty[168 chars]'', 'url': '/pages/', 'attrs': {}}]}], [{'_typ[461 chars]m'}]}
Diff is 1499 characters long. Set self.maxDiff to None to see it.

My guess is the URL generation has changed for.the sidebar data, maybe this is not intentional.

Also, you'll need to do a make format to ensure the CI format checks pass.

Great stuff - myself or someone will do a full review when possible.

@nandini584
Copy link
Contributor Author

Ohk...

@lb-
Copy link
Member

lb- commented Mar 26, 2024

Ohh wait, I think we DO want to change the default (re-reading the previous PR), so the failed test may need to be updated to reflect the new behaviour.

@lb-
Copy link
Member

lb- commented Mar 26, 2024

@nandini584 just checking, it would be good for you to also have a full read of the previous PR's comments. I did a review of that suggesting a different approach where we do this change but also support reading any other default params from the configured (in settings) URL.

@nandini584
Copy link
Contributor Author

Sure @lb- , I will have a full read and make updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants