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: Send email when change in session state #7157

Merged
merged 3 commits into from
Jul 24, 2020

Conversation

Haider8
Copy link
Contributor

@Haider8 Haider8 commented Jul 23, 2020

Fixes #7155

Short description of what this resolves:

Email was being sent only when the final session state is accepted or rejected.

Changes proposed in this pull request:

Send email when there's a change in session state.

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@Haider8 Haider8 requested a review from iamareebjamal July 23, 2020 09:17
@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #7157 into development will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #7157   +/-   ##
============================================
  Coverage        62.84%   62.85%           
============================================
  Files              262      262           
  Lines            13055    13056    +1     
============================================
+ Hits              8205     8206    +1     
  Misses            4850     4850           
Impacted Files Coverage Δ
app/api/sessions.py 55.41% <100.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 635bca8...5c68360. Read the comment docs.

@iamareebjamal
Copy link
Member

That's even worse. Read the issue again

@Haider8
Copy link
Contributor Author

Haider8 commented Jul 23, 2020

We want to send email if there's any change in session details. Then we won't be checking if there's 'state' in data?

@iamareebjamal
Copy link
Member

iamareebjamal commented Jul 23, 2020

That's the opposite of what we want. Why would we want that? Read the issue again carefully

new_state is not None
and new_state != session.state
and (new_state == 'accepted' or new_state == 'rejected')
)
Copy link
Member

Choose a reason for hiding this comment

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

This should be written inside the if condition below

Copy link
Member

Choose a reason for hiding this comment

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

This also does not contain the data.get('send_email') logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So below I should write (new_state == 'accepted' or new_state == 'rejected') and data.get('send_email', None) apart from g.send_email?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

@niranjan94
Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ app/api/sessions.py  -1
         

See the complete overview on Codacy

@Haider8 Haider8 requested a review from iamareebjamal July 24, 2020 10:14
@iamareebjamal iamareebjamal changed the title Send email when change in session state fix: Send email when change in session state Jul 24, 2020
@auto-label auto-label bot added the fix label Jul 24, 2020
@iamareebjamal iamareebjamal merged commit 4fc2574 into fossasia:development Jul 24, 2020
@Haider8 Haider8 deleted the issue-7155 branch July 24, 2020 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session accept reject email sent without state changes
3 participants