Skip to content

Conversation

@estyxx
Copy link
Member

@estyxx estyxx commented Dec 1, 2024

What

ToDo

@vercel
Copy link

vercel bot commented Dec 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
pycon ❌ Failed (Inspect) Dec 1, 2024 5:32pm

@codecov
Copy link

codecov bot commented Dec 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.12%. Comparing base (86af124) to head (1116b46).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4195      +/-   ##
==========================================
+ Coverage   94.07%   94.12%   +0.05%     
==========================================
  Files         331      331              
  Lines        9159     9176      +17     
  Branches     1347      627     -720     
==========================================
+ Hits         8616     8637      +21     
  Misses        453      453              
+ Partials       90       86       -4     

@estyxx estyxx marked this pull request as ready for review December 1, 2024 13:46

if len(value) > max_length:
if value and len(value) > max_length:
print(field)
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 remove this print?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

blank=True,
null=True,
)
departure_city = models.CharField(
Copy link
Member

Choose a reason for hiding this comment

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

Only feedback is that it would be good to a bit more consistent when naming fields.

travelling from and departure city are both connected but have such different names, maybe it would have been a bit better to have more similar names, e.g. travellingFromCountry travellingFromCity so that the fact that they are connected is explicit. Or, rename travelling from to departureCountry

We can keep it as it is for this PR and we can discuss a refactor in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can change it in this PR, so we don't produce unnecessary migrations...

"grants.form.fields.travellingFrom.description":
"Indica la città e il paese da cui partirai per partecipare alla conferenza.",
"Seleziona il paese da cui partirai per partecipare alla conferenza.",
"grants.form.fields.departureCity": "Qual è la tua nazionalità?",
Copy link
Member

Choose a reason for hiding this comment

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

This copy seems incorrect

Copy link
Member

@marcoacierno marcoacierno left a comment

Choose a reason for hiding this comment

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

The italian copy is incorrect. Once fixed this should be ok to merge

- Made `travelling_from` and `departure_city` fields mandatory only if `needsFundsForTravel` is set to "true".
- Updated UI to hide these fields by default and show them only when `needsFundsForTravel` is "true".
- Modified API to handle these fields as optional.
The email field is no longer required in the grant form,
so the corresponding translations have been removed from the locale files.
@estyxx estyxx requested a review from marcoacierno December 1, 2024 16:40
@estyxx estyxx merged commit 8ede0e7 into main Dec 1, 2024
6 of 8 checks passed
@estyxx estyxx deleted the grants/add-travel-fields branch December 1, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants