Skip to content
Snippets Groups Projects

Fix accepted mail sending when present in eventsignups POST

Merged Imported Johannes Zumthurm requested to merge fix-415-eventsignup-email into master

Created by: temparus

Ensures that an accepted email is sent to the user when an admin created an event signup directly with accepted = true.

Fixes #415 (closed).

Merge request reports

Checking pipeline status.

Approval is optional

Merged by Johannes ZumthurmJohannes Zumthurm 1 year ago (Dec 17, 2023 7:04pm UTC)

Merge details

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
234 """Test that an accepted email is sent when an admin has added a
235 signup manually."""
236 event = self.new_object('events', spots=1, selection_strategy='fcfs',
237 allow_email_signup=True)
238
239 user = self.new_object('users')
240
241 self.api.post('/eventsignups', data={
242 'user': str(user['_id']),
243 'event': str(event['_id']),
244 'accepted': True
245 }, token=self.get_root_token(), status_code=201).json
246
247 mail = self.app.test_mails[0]
248 for field in mail.values():
249 self.assertTrue('None' not in field)
  • Created by: NotSpecial

    Does it have to be 'None' and not just None? (Not sure, I am not familiar with the mail tests, sorry).

  • Created by: temparus

    We have to check for the string 'None' to make sure that neither the subject nor the email body contain that phrase as it would indicate that some placeholder value was not set properly.

  • Created by: NotSpecial

    And subjet and body are guaranteed to be strings, they can never be None?

  • Created by: temparus

    I'm not sure, but the existing tests assumed that. So I guess that it is guaranteed.

  • Created by: NotSpecial

    I did not write the mail test so I am not familiar with this. Could you double check this please?

    In particular, can you check why it's 'None' (string) not not just None? If we know the reason I am happy to acccapt it, but like this its a bit shady.

  • Created by: temparus

    Well, the test case with the None values in the templated string fields is pretty obvious to me. Nevertheless I just added a comment there which additionally explains it in English.

    So to explain it again with an example: A confirmation email for the event signup of event XYZ should be sent. This is a prepared message containing placeholders. So the event name is a placeholder which gets replaced by 'XYZ'. In case the event name is None for some weird reason, the event name then listed in the message is 'None'.

  • Created by: suisseWalter

    this looks like a satisfactory explanation to me.

  • Author Owner

    Created by: NotSpecial

    Review: Commented

    Thanks for the updates. I have just one comment on the test because I am unfamiliar with the mail-related tests.

  • Author Owner

    Created by: temparus

    I just noticed that the accepted email was sent by the api already, so it is actually a non-issue. But I still think that this additional test to check that behavior might be helpful nonetheless.

  • Author Owner

    Created by: codecov[bot]

    Codecov Report

    All modified and coverable lines are covered by tests :white_check_mark:

    Comparison is base (ece574e) 94.76% compared to head (3b45865) 94.77%.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##           master     #416      +/-   ##
    ==========================================
    + Coverage   94.76%   94.77%   +0.01%     
    ==========================================
      Files          75       75              
      Lines        4315     4329      +14     
    ==========================================
    + Hits         4089     4103      +14     
      Misses        226      226              
    Flag Coverage Δ
    unittests 94.77% <100.00%> (+0.01%) :arrow_up:

    Flags with carried forward coverage won't be shown. Click here to find out more.

    :umbrella: View full report in Codecov by Sentry.
    :loudspeaker: Have feedback on the report? Share it here.

  • Johannes Zumthurm requested review from @jzumthurm · Imported

    requested review from @jzumthurm

  • Johannes Zumthurm
    Johannes Zumthurm @jzumthurm started a thread on commit 45023710
  • 252
    253 self.api.post('/eventsignups', data={
    254 'email': 'a@example.com',
    255 'event': str(event['_id']),
    256 'accepted': True
    257 }, token=self.get_root_token(), status_code=201).json
    258
    259 # accepted email and confirmation email should be sent.
    260 self.assertIs(len(self.app.test_mails), 3)
    261 for mail in self.app.test_mails[1:]:
    262 for field in mail.values():
    263 self.assertTrue('None' not in field)
    264
    265 self.assertTrue(
    266 ('accepted' in mail['subject']) or
    267 ('confirm_email' in mail['text']))
    • Created by: NotSpecial

      Arg, a bit the same issue as above. These assertions seem super fishy. I guess it's because of the way the test is set up? Maybe we can add a comment to explain why asserting this means that everything works?

    • Created by: temparus

      This check is indeed not very clear as it is written. Therefore I just added an explanation as a comment in the code.

  • Johannes Zumthurm mentioned in issue #493 (closed) · Imported

    mentioned in issue #493 (closed)

  • Johannes Zumthurm requested review from @jzumthurm · Imported

    requested review from @jzumthurm

  • Johannes Zumthurm requested review from @jzumthurm · Imported

    requested review from @jzumthurm

  • Author Owner

    Created by: suisseWalter

    Review: Approved

    This testcase looks good to me. and as it just adds a testcase for something that hasn't been tested in the past it should be fine.

  • Author Owner

    Merged by: temparus at 2023-12-17 19:04:33 UTC

  • Johannes Zumthurm merged manually · Imported

    merged manually

  • Johannes Zumthurm closed · Imported

    closed

  • Please register or sign in to reply
    Loading