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

Fix email issues #171

Merged
merged 28 commits into from
Oct 5, 2021
Merged

Fix email issues #171

merged 28 commits into from
Oct 5, 2021

Conversation

jrha
Copy link
Contributor

@jrha jrha commented Aug 14, 2019

Depends on #238

Fixes #64, fixes #163 and fixes #168.

@jrha jrha force-pushed the emails branch 4 times, most recently from 4e942cd to 690aa0f Compare August 15, 2019 11:33
@jrha jrha force-pushed the emails branch 2 times, most recently from b4d9057 to e93cd6d Compare August 26, 2020 10:42
@jrha jrha marked this pull request as ready for review August 26, 2020 10:47
@jrha jrha requested a review from gregcorbett November 9, 2020 10:58
@gregcorbett gregcorbett added this to the 5.8.0 milestone Apr 23, 2021
@jrha jrha requested a review from a team as a code owner May 19, 2021 15:33
@gregcorbett
Copy link
Member

I've added a few commits that fix some bugs I found while trying to put together a sanity check that this was working as I would expect.

@gregcorbett
Copy link
Member

gregcorbett commented May 20, 2021

I've had my hands all over this now, so I'd appreciate a review from @tofu-rocketry or @ineilson.

For my own future reference:

@gregcorbett
Copy link
Member

Looking at Travis - I should merge #238 first.

lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
@gregcorbett gregcorbett force-pushed the emails branch 5 times, most recently from f64884e to 3aabc41 Compare August 26, 2021 16:01
@gregcorbett
Copy link
Member

gregcorbett commented Aug 26, 2021

As a note to Future Greg, one of these commits reverts another one of these commits - that at least can be squashed.

One conversation currently unresolved needing input from @ineilson. Resolved.

@gregcorbett
Copy link
Member

As a further note to Future Greg, print_r is also discouraged by codacy - but in a less severe way... so progress?

ghost
ghost previously approved these changes Sep 8, 2021
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
DeckChairs and others added 3 commits September 30, 2021 16:27
* Add the entity the role was requested on into the message
* Add context and personalise role request emails

Co-authored-by: ineilson <[email protected]>
@gregcorbett
Copy link
Member

gregcorbett commented Sep 30, 2021

I've squashed some of my more "bug-fixy" commits into other commits but decided to leave the majority to not conflate moving code about and changing behaviour.

As a further note to Future Greg, print_r is also discouraged by codacy - but in a less severe way... so progress?

Just this and a final test (can it still send emails after the rebase) between us and GOCDB 5.8! DONE!

jrha and others added 11 commits October 1, 2021 12:46
This seems to have originally been factored out as part of 229a0e8
and has been sitting around since then.
- this way, if a site role request gets passed up to an NGI level
  user, they will see the site name in the request (not the NGI
  name).
- I guess this logic was somewhat there to test /stop recursion
  if you didn't want to send an email.
- The logic of whether to send an email or just print what would
  have been sent has been factored out already, so we don't need
  this if statement anymore.
- to support different views of GOCDB being served by the same instance.

Co-authored-by: ineilson <[email protected]>
- the behaviour of recursing this function is repeated further
  down.
- `if ( empty( $array_name ) )` is more readable. I think we
  previously got away with it because `[] == null` returns true,
  or because the code interchanged empty arrays and `null`.
@gregcorbett
Copy link
Member

@ineilson could I get your seal of approval? 🦭

ghost
ghost previously approved these changes Oct 4, 2021
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
lib/Gocdb_Services/NotificationService.php Outdated Show resolved Hide resolved
gregcorbett and others added 7 commits October 4, 2021 12:39
- this check is part of the `else` of an
  `if (count($authorisingUserIds) == 0)` statement.
- `array_unique` won't empty `$authorisingUserIds`.
- hence, the check here isn't needed.

Co-authored-by: ineilson <[email protected]>
@gregcorbett
Copy link
Member

Suggestions applied, @ineilson could I get a fresh seal of approval?

@gregcorbett gregcorbett merged commit 815b777 into GOCDB:dev Oct 5, 2021
gregcorbett added a commit to ElliottKasoar/gocdb that referenced this pull request Feb 8, 2022
gregcorbett added a commit to gregcorbett/gocdb that referenced this pull request May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants