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

💫 feat: change infrastructure condition field to ManyToMany field (#3970) #4084

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

juggler31
Copy link
Contributor

@juggler31 juggler31 commented Apr 26, 2024

Description

Related Issue

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

@juggler31 juggler31 changed the title feat: change infrastructure condition field to ManyToMany field (#3970) 🐛 feat: change infrastructure condition field to ManyToMany field (#3970) Apr 26, 2024
@juggler31 juggler31 changed the title 🐛 feat: change infrastructure condition field to ManyToMany field (#3970) 💫 feat: change infrastructure condition field to ManyToMany field (#3970) Apr 26, 2024
@juggler31 juggler31 linked an issue Apr 26, 2024 that may be closed by this pull request
@juggler31 juggler31 force-pushed the state_amenagement_toM2M branch 2 times, most recently from 23530e8 to 1e6dfd9 Compare April 26, 2024 15:03
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.90%. Comparing base (95b1c8b) to head (1197850).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4084      +/-   ##
==========================================
- Coverage   98.38%   95.90%   -2.48%     
==========================================
  Files         267      267              
  Lines       20810    20819       +9     
==========================================
- Hits        20473    19966     -507     
- Misses        337      853     +516     

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

Copy link

cypress bot commented Apr 26, 2024

Passing run #9592 ↗︎

0 22 0 0 Flakiness 0

Details:

Merge 1197850 into 7fa9588...
Project: Geotrek-admin Commit: d0a0c2c0ff ℹ️
Status: Passed Duration: 01:58 💡
Started: Jul 19, 2024 3:39 PM Ended: Jul 19, 2024 3:41 PM

Review all test suite changes for PR #4084 ↗︎

@juggler31 juggler31 requested a review from a team April 26, 2024 16:32
{% else %}
<span>{{ condition.label }}<span>
{% endif %}
{% endfor %}
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use {{ object.conditions_display }} ?

if extracted:
obj.conditions.set(extracted)
else:
obj.conditions.add(InfrastructureConditionFactory.create())
Copy link
Member

Choose a reason for hiding this comment

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

can you factorize post_generation in an InfrastructureFatoyMixin ?

@@ -21,7 +21,9 @@ CREATE VIEW {{ schema_geotrek }}.v_infrastructures AS WITH v_infra AS
{% endfor %}
CONCAT (e.min_elevation, 'm') AS altitude,
t.implantation_year,
t.condition_id,
{% for conditon in t.conditions %}
conditon_id,
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you add each id in a dedicated column


# Copy InfrastructureCondition to SignageCondition and BladeCondition
for infrastructure in Infrastructure.objects.all():
infrastructure.conditions.add(infrastructure.condition)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fails if we have an infrastructure with no Condition

@juggler31 juggler31 force-pushed the state_amenagement_toM2M branch 2 times, most recently from d262e54 to cc3bf46 Compare May 2, 2024 14:41
@juggler31 juggler31 force-pushed the state_amenagement_toM2M branch 4 times, most recently from 59bd5c9 to 6059086 Compare May 17, 2024 15:26
conditions = models.ManyToManyField(
InfrastructureCondition,
related_name='infrastructures',
verbose_name=_("Condition"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
verbose_name=_("Condition"),
verbose_name=_("Conditions"),

geotrek/infrastructure/models.py Show resolved Hide resolved
@juggler31 juggler31 force-pushed the state_amenagement_toM2M branch 2 times, most recently from 7a7e7f5 to 164072f Compare July 19, 2024 09:05
@@ -5,6 +5,9 @@ CHANGELOG
2.108.0+dev (XXXX-XX-XX)
----------------------------

**Improvements**

- Change infrastructure condition field to ManyToMany field (#3970)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Change infrastructure condition field to ManyToMany field (#3970)
- Change infrastructure condition field to ManyToMany field (#3970)

@@ -78,6 +81,7 @@ https://geotrek.readthedocs.io/en/latest/install/upgrade.html#postgresql
- Bump mapentity to 8.9.0



Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@@ -129,7 +133,6 @@ https://geotrek.readthedocs.io/en/latest/install/upgrade.html#postgresql
- Add structures to Aggregator (#3569)
- Allow to filter flatpages by portal on admin list page

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@juggler31 juggler31 force-pushed the state_amenagement_toM2M branch 2 times, most recently from 6b7b37d to a60e224 Compare July 19, 2024 14:11
@juggler31 juggler31 requested a review from submarcos July 19, 2024 15:26
@juggler31 juggler31 merged commit 7d2e100 into master Jul 23, 2024
20 of 21 checks passed
@juggler31 juggler31 deleted the state_amenagement_toM2M branch July 23, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibilité de sélectionner plusieurs état sur les aménagements
4 participants