Skip to content

fix: Allow more email addresses in notification lists#4792

Merged
rjsparks merged 7 commits into
ietf-tools:mainfrom
larseggert:fix-4736
Dec 8, 2022
Merged

fix: Allow more email addresses in notification lists#4792
rjsparks merged 7 commits into
ietf-tools:mainfrom
larseggert:fix-4736

Conversation

@larseggert

Copy link
Copy Markdown
Collaborator

Fixes #4736

@rjsparks rjsparks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is insufficient - a matching change to the document model is also needed, or the extra bits won't be stored in the database. See https://github.com/ietf-tools/datatracker/blob/main/ietf/doc/models.py#L111.

It's not immediately clear whether leaving this as a CharField in the model and making it significantly larger is the right thing to do. I suspect it probably needs to be converted to a TextField. Either way, the change will need to be accompanied by a migration effecting the change in the database.

(You build that with something like ietf/maange.py makemigrations doc after changing the model.)

@larseggert larseggert requested a review from rjsparks December 5, 2022 14:41
Comment thread ietf/doc/migrations/send_notices.py Outdated
@larseggert larseggert requested a review from rjsparks December 5, 2022 18:01
@codecov

codecov Bot commented Dec 5, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4792 (c7643bd) into main (9457e2b) will increase coverage by 0.00%.
The diff coverage is 82.75%.

@@           Coverage Diff           @@
##             main    #4792   +/-   ##
=======================================
  Coverage   88.45%   88.46%           
=======================================
  Files         296      296           
  Lines       39742    39752   +10     
=======================================
+ Hits        35155    35166   +11     
+ Misses       4587     4586    -1     
Impacted Files Coverage Δ
ietf/utils/draft.py 71.75% <54.54%> (+0.11%) ⬆️
ietf/doc/forms.py 96.00% <100.00%> (ø)
ietf/doc/models.py 88.44% <100.00%> (ø)
ietf/doc/views_conflict_review.py 96.75% <100.00%> (ø)
ietf/doc/views_draft.py 91.96% <100.00%> (ø)
ietf/doc/views_status_change.py 93.38% <100.00%> (ø)
ietf/liaisons/forms.py 91.21% <100.00%> (ø)
ietf/meeting/helpers.py 89.87% <100.00%> (+0.04%) ⬆️
ietf/meeting/views.py 90.56% <100.00%> (+0.01%) ⬆️
ietf/doc/views_search.py 89.23% <0.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jennifer-richards jennifer-richards left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only comment is that I'd probably have named the migration something more descriptive (0048_lengthen_notification_lists or something), but that's a small nit. Looks good otherwise.

@rjsparks rjsparks merged commit f870715 into ietf-tools:main Dec 8, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 12, 2022
@larseggert larseggert deleted the fix-4736 branch December 15, 2022 15:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maximum number of email addresses on send notices to

3 participants