Respond to Reviews CR#7825
Merged
sloria merged 3 commits intoOct 19, 2017
Merged
Conversation
laurenbarker
force-pushed
the
improvement/respond-cr
branch
2 times, most recently
from
October 19, 2017 18:21
0071281 to
52ae276
Compare
Contributor
Author
|
@sloria this is ready for review 🐧 |
sloria
reviewed
Oct 19, 2017
| subject='Share Error' | ||
| ) | ||
|
|
||
| REVIEWS_SUBMISSION_CONFIRMATION = lambda provider_name: Mail( |
Contributor
There was a problem hiding this comment.
The lambda shouldn't be necessary. The subject can be a string template that receives the same context as the email body.
For example
FILE_OPERATION_SUCCESS = Mail(
'file_operation_success',
subject='Your ${action} has finished',
)
Rename migrations Use variable for related node Use constant in mails.py Change message field on NotificationDigest from char to text Add management command for adding subscriptions to users
laurenbarker
force-pushed
the
improvement/respond-cr
branch
from
October 19, 2017 18:54
52ae276 to
1e82ff7
Compare
sloria
suggested changes
Oct 19, 2017
sloria
left a comment
sloria
left a comment
Contributor
There was a problem hiding this comment.
Just one nitpick to address
sloria
reviewed
Oct 19, 2017
| user_subscriptions = get_user_subscriptions(user, event_type) | ||
| context['no_future_emails'] = user_subscriptions['none'] | ||
| context['is_creator'] = user == context.get('reviewable').node.creator | ||
| context['provider_name']= context.get('reviewable').provider.name |
Contributor
There was a problem hiding this comment.
Is reviewable guaranteed to be in the context?
If so, then context['reviewable'] is preferred, to make it clear that it's guaranteed.
If not, then this code will need to be changed to handle the case when it's not there.
laurenbarker
force-pushed
the
improvement/respond-cr
branch
2 times, most recently
from
October 19, 2017 20:29
d5bdee5 to
37b6874
Compare
laurenbarker
force-pushed
the
improvement/respond-cr
branch
from
October 19, 2017 20:30
37b6874 to
25e9014
Compare
sloria
approved these changes
Oct 19, 2017
Contributor
|
OK, stage3 is now rolled back to 0060. Let's move on.. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this is merged let Steve know so he can rollback the staging3 database to
0060_reviewsto prevent the name changes from breaking things.Staging3 doesn't have very many users but if needed,
0061_add_reviews_notification_subscriptioncan be skipped/faked and subscriptions can be added usingpython manage.py add_notification_subscription --notification global_reviews.Purpose
Respond to CR on #7807.
Note that this PR doesn't update the myProjects page to use 2.6. I think that would need a regression test from QA which is out of scope of reviews changes.
Changes
messagefield onNotificationDigesttoTextFieldadd_notification_subscription --notification global_reviewsSide effects
Probably messes up your migration history. I just deleted the relevant migration history from
django_migrationstable using DataGrip and reranmigrate.**Probably not the recommended way to handle inconsistent history.