Skip to content

Conversation

@uds5501
Copy link
Contributor

@uds5501 uds5501 commented Jun 21, 2019

Fixes #6085

Changes proposed in this pull request:

  • modify both ticket cancellation notifications
  • add links as required.

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.

Notifications after the change :

Screenshot from 2019-06-21 21-41-37

@auto-label auto-label bot added the feature label Jun 21, 2019
@uds5501 uds5501 force-pushed the cancel-notif-change branch from 9597d90 to f22b484 Compare June 21, 2019 16:22
@codecov
Copy link

codecov bot commented Jun 21, 2019

Codecov Report

Merging #6084 into development will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #6084      +/-   ##
==============================================
+ Coverage        66.17%   66.2%   +0.03%     
==============================================
  Files              285     285              
  Lines            14133   14134       +1     
==============================================
+ Hits              9353    9358       +5     
+ Misses            4780    4776       -4
Impacted Files Coverage Δ
app/api/helpers/system_notifications.py 100% <ø> (ø) ⬆️
app/api/helpers/notification.py 21.97% <100%> (+0.86%) ⬆️
app/api/helpers/scheduled_jobs.py 26.59% <0%> (+4.25%) ⬆️

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 e812d43...f22b484. Read the comment docs.

@uds5501
Copy link
Contributor Author

uds5501 commented Jun 26, 2019

@mrsaicharan1 @shreyanshdwivedi Please review!

@uds5501
Copy link
Contributor Author

uds5501 commented Jun 27, 2019

@CosmicCoder96 @iamareebjamal Please take a look?

Copy link
Member

@mrsaicharan1 mrsaicharan1 left a comment

Choose a reason for hiding this comment

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

I believe that cancelled/expired tickets don't have any vacancy. How is it that you're planning for that

@uds5501
Copy link
Contributor Author

uds5501 commented Jun 27, 2019

@mrsaicharan1 I don't think we need to do anything for that. The issue required a ticket link and I provided that. A cancelled order having no ticket holders is as it is supposed to be.

Any cancelled ticket should be relocated so that it's available for sale.

@mrsaicharan1
Copy link
Member

@mrsaicharan1 I don't think we need to do anything for that. The issue required a ticket link and I provided that. A cancelled order having no ticket holders is as it is supposed to be.

Any cancelled ticket should be relocated so that it's available for sale.

Agreed

Copy link
Member

@mrsaicharan1 mrsaicharan1 left a comment

Choose a reason for hiding this comment

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

LGTM

@iamareebjamal iamareebjamal merged commit ed56f22 into fossasia:development Jun 27, 2019
iamareebjamal pushed a commit to iamareebjamal/open-event-server that referenced this pull request Aug 2, 2019
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.

Cancelled Order notifications should have link to the order page itself

5 participants