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.

Sign up for GitHub

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

Jump to bottom

add price to TouristicEvent - ref #3587 #3855

Closed
wants to merge 3 commits into from
Closed

add price to TouristicEvent - ref #3587 #3855

wants to merge 3 commits into from

Conversation

TheoLechemia
Copy link
Contributor

@TheoLechemia TheoLechemia commented Dec 14, 2023 •
edited
Loading

Description

Related Issue

#3587

Checklist

  • I have followed the guidelines in our Contributing document
  • My code respects the Definition of done available in the Development section of the documentation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes
  • I added an entry in the changelog file
  • My commits are all using prefix convention (emoji + tag name) and references associated issues
  • I added a label to the PR corresponding to the perimeter of my contribution
  • The title of my PR mentionned the issue associated

@cypress Cypress
Copy link

cypress bot commented Dec 14, 2023 •
edited
Loading

Passing run #8109 ↗︎

0 24 0 0 Flakiness 0

Details:

Merge ec60c09 into ad649e5...
Project: Geotrek-admin Commit: 6db2ec847d ℹ️
Status: Passed Duration: 02:17 💡
Started: Feb 28, 2024 3:22 PM Ended: Feb 28, 2024 3:25 PM

Review all test suite changes for PR #3855 ↗︎

@codecov Codecov
Copy link

codecov bot commented Dec 14, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.35%. Comparing base (ad649e5) to head (ec60c09).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3855   +/-   ##
=======================================
  Coverage   98.35%   98.35%           
=======================================
  Files         294      294           
  Lines       22292    22293    +1     
=======================================
+ Hits        21925    21926    +1     
  Misses        367      367

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheoLechemia TheoLechemia marked this pull request as draft December 15, 2023 08:33
@TheoLechemia TheoLechemia force-pushed the feat_touristic_event_price branch 3 times, most recently from d1a1bbc to 1c76ad8 Compare December 19, 2023 15:01
@TheoLechemia TheoLechemia marked this pull request as ready for review January 15, 2024 10:57
@TheoLechemia TheoLechemia force-pushed the feat_touristic_event_price branch from 1c76ad8 to 73269bb Compare February 14, 2024 14:36
@TheoLechemia TheoLechemia force-pushed the feat_touristic_event_price branch 2 times, most recently from b7d9e2a to c80feee Compare February 23, 2024 16:20
marcantoinedupre
marcantoinedupre reviewed Feb 26, 2024
View reviewed changes
geotrek/tourism/models.py Outdated
@@ -458,6 +458,13 @@ class TouristicEvent(ZoningPropertiesMixin, AddPropertyMixin, PublishableMixin,
help_text=_("In hours (1.5 = 1 h 30, 24 = 1 day, 48 = 2 days)"),
validators=[MinValueValidator(0)]
)
price = models.FloatField(
Copy link
Contributor

Choose a reason for hiding this comment

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

I advise you to consider a DecimalField here. It will prevent many rounding issues coming with floating numbers. It is not mandatory in your case since there is no computation on that field. But I think it would guarantee we won't have to deal with rounding and/or formatting to display the values properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can rebase if necessary

@TheoLechemia TheoLechemia force-pushed the feat_touristic_event_price branch 3 times, most recently from 1ba104c to c4444c2 Compare February 26, 2024 11:07
submarcos
submarcos requested changes Feb 28, 2024
View reviewed changes
Copy link
Member

@submarcos submarcos left a comment

Choose a reason for hiding this comment

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

Je pense qu'il y a un probleme avec tes traductions car toutes les trads venant de geotrek/locales/ (trads non incluses dans des apps) ont enormément de changements
https://github.com/GeotrekCE/Geotrek-admin/pull/3855/files#diff-fc6029e51be9d381ebe39b2b526f0a10f7cb10fd0ed1800e0a79048e8dbe58d3

Je pense que t'as du laisser un venv et qu'il est interprété lors du make messages

@TheoLechemia TheoLechemia force-pushed the feat_touristic_event_price branch from 1226602 to ec60c09 Compare February 28, 2024 15:10
@TheoLechemia
Copy link
Contributor Author

J'ai relancé le make message et fixup le commit des traduction.
Pareil dites moi si je dois squasher

@submarcos
Copy link
Member

replaced by #3966 3966

@submarcos submarcos closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcantoinedupre marcantoinedupre marcantoinedupre left review comments

@camillemonchicourt camillemonchicourt camillemonchicourt left review comments

@submarcos submarcos submarcos requested changes

Assignees
No one assigned
Labels
✨ Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TheoLechemia @submarcos @camillemonchicourt @marcantoinedupre