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: allow refund_policy to be none #7254

Merged
merged 3 commits into from Sep 10, 2020

Conversation

snitin315
Copy link
Member

@auto-label auto-label bot added the fix label Sep 10, 2020
refund_policy = db.Column(
db.String, default='All sales are final. No refunds shall be issued in any case.'
)
refund_policy = db.Column(db.String)
Copy link
Member Author

Choose a reason for hiding this comment

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

we need a migration for past events, right?

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #7254 into development will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #7254   +/-   ##
============================================
  Coverage        63.55%   63.55%           
============================================
  Files              259      259           
  Lines            13034    13034           
============================================
  Hits              8284     8284           
  Misses            4750     4750           
Impacted Files Coverage Δ
app/api/schema/events.py 96.89% <100.00%> (ø)
app/models/event.py 77.47% <100.00%> (ø)

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 97f3497...bd63158. Read the comment docs.

@iamareebjamal
Copy link
Member

Yes, we need to set refund policy to null for previous events.

@iamareebjamal
Copy link
Member

Thinking about it now, if some user changed their refund policy using the API, making it null will lead to data loss. So, wait for Mario to tell what should be done

@snitin315
Copy link
Member Author

Thinking about it now, if some user changed their refund policy using the API, making it null will lead to data loss. So, wait for Mario to tell what should be done

I think we can apply migration only to events having the default refund_policy message.

@mariobehling
Copy link
Member

In regards to events we should delete the policy. There was not any organizer who edited the policy, we just introduced it a few hours ago.

Is that easy to do? I dont think we need migrations.

@iamareebjamal
Copy link
Member

iamareebjamal commented Sep 10, 2020

We didn't add it a few hours ago. We used it as default refund policy for all events in the backend since a long time ago

@iamareebjamal iamareebjamal merged commit 5bc7012 into fossasia:development Sep 10, 2020
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 10, 2020

This pull request introduces 2 alerts when merging bd63158 into 97f3497 - view on LGTM.com

new alerts:

  • 2 for Unused import

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.

None yet

3 participants