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

Issue #3373122 by SV: Users profile images not shown in email notifications #3459

Merged
merged 2 commits into from Jul 18, 2023

Conversation

volodymyr-sydor
Copy link
Contributor

@volodymyr-sydor volodymyr-sydor commented Jul 7, 2023

Problem

Currently when receiving emails with notifications, for example when someone joined a group It will show a broken profile image

Solution

Looks like lazyload causes that issue and needs to disable in the related email template
Also, if image is not public then need to show default placeholder image

Issue tracker

Theme issue tracker

N/A

How to test

  • Using the latest version of Open Social with the lazy-related modules enabled
  • As a sitemanager
  • Try to create a flexible group
  • Then, with a second account, join this group
  • When I receive notification emails I expect to see a user profile image but instead see a broken one.
  • The expected result is attained when repeating the steps with this fix applied.

Definition of done

Before merge

  • Code/peer review is completed
  • All commit messages are clear and clean. If applicable a rebase was performed
  • All automated tests are green
  • Functional/manual tests of the acceptance criteria are approved
  • All acceptance criteria were met
  • New features or changes to existing features are covered by tests, either unit (preferably) or behat
  • Update path is tested. New hook_updates should respect update order, right naming convention and consider hook_post_update code
  • Module can be safely uninstalled. Update/implement hook_uninstall and make sure that removed configuration or dependencies are removed/uninstalled
  • This pull request has all required labels (team/type/priority)
  • This pull request has a milestone
  • This pull request has an assignee (if applicable)
  • Any front end changes are tested on all major browsers
  • New UI elements, or changes on UI elements are approved by the design team
  • New features, or feature changes are approved by the product owner

After merge

  • Code is tested on all branches that it has been cherry-picked
  • Update hook number might need adjustment, make sure they have the correct order
  • The Drupal.org ticket(s) are updated according to this pull request status

Screenshots

Before
Screenshot 2023-07-07 at 12 14 56

After:
image

Release notes

Fixes an issue with broken profile image in the email notification

Change Record

N/A

Translations

N/A

@volodymyr-sydor volodymyr-sydor added type: bug Fixes a bug in Open Social team: enterprise This PR originates from the ECI team status: needs work This pull request needs more work before it's ready for review prio: medium team: finalizer labels Jul 7, 2023
@volodymyr-sydor volodymyr-sydor added this to the 11.10.0-alpha1 milestone Jul 7, 2023
@volodymyr-sydor volodymyr-sydor self-assigned this Jul 7, 2023
@mergeable
Copy link

mergeable bot commented Jul 7, 2023

Thanks for contributing towards Open Social! A maintainer from the @goalgorilla/maintainers group might not review all changes from all teams/contributors. Please don't be discouraged if it takes a while. In the meantime, we have some automated checks running and it might be that you will see our comments with some tips or requests to speed up the review process. 😊

@volodymyr-sydor volodymyr-sydor force-pushed the bugfix/3373122-profile-image branch 5 times, most recently from b161d19 to 94df78b Compare July 10, 2023 16:58
@volodymyr-sydor volodymyr-sydor added status: needs review This pull request is waiting for a requested review and removed status: needs work This pull request needs more work before it's ready for review labels Jul 11, 2023
@volodymyr-sydor volodymyr-sydor marked this pull request as ready for review July 11, 2023 13:21
Copy link
Contributor

@ribel ribel left a comment

Choose a reason for hiding this comment

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

Changes were tested and looks good to me.

@ribel ribel modified the milestones: 11.10.0-beta1, 11.9.7 Jul 17, 2023
Copy link
Contributor

@tbsiqueira tbsiqueira left a comment

Choose a reason for hiding this comment

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

Code is approved, let's wait for functional tests

Copy link
Contributor

@tbsiqueira tbsiqueira left a comment

Choose a reason for hiding this comment

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

Is it possible to create behat tests for this? We have access to the email content

@volodymyr-sydor
Copy link
Contributor Author

Is it possible to create behat tests for this? We have access to the email content

theoretically yes, but I guess could be some "pitfalls" because here we needed an email with an attachment
I will check and if wouldn't any blocker I'll try to add it

@ribel ribel merged commit dd52812 into main Jul 18, 2023
191 checks passed
@ribel ribel deleted the bugfix/3373122-profile-image branch July 18, 2023 12:50
@ribel ribel added backport: verified This pull request has been back ported to an older minor version status: no action needed and removed status: needs review This pull request is waiting for a requested review labels Jul 18, 2023
@ribel
Copy link
Contributor

ribel commented Jul 18, 2023

🍒 picked to 11.10.x and 11.9.x (conflicts were resolved)

@volodymyr-sydor
Copy link
Contributor Author

Is it possible to create behat tests for this? We have access to the email content

I've checked it and faced with a few issues/blockers:

  • currently, a notification-emails.feature is disabled for some reason
  • we don't have any other similar test scenario for join roup (only invitation) that we can adjust
  • the current method findSubjectAndBody() checks only the subject and skip a body
  • and if we try to get a email about joining group that email will be without an attachment

So, looks like for the current moment it should be postponed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport: verified This pull request has been back ported to an older minor version prio: medium team: enterprise This PR originates from the ECI team type: bug Fixes a bug in Open Social
3 participants