Use the Notification model when viewing a single notification
#5318
Conversation
This file contains 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
quis
changed the title
Notification model when viewing a single notificationNotification model when viewing a single notification
quis
force-pushed
the
refactor-single-notification-into-model
branch
from
b7ae5de to
2d0f5bb
Compare
This continues the work done in https://github.com/alphagov/notifications-admin/pull/5232/files Notifications are one of the last database objects in the admin app that we are still passing around as raw JSON, rather than wrapping them in a proper Python object.
Generally it’s a clearer separation of concerns if the view doesn’t talk to the raw API client method, but instead uses a method or property on the model. This also reduces the number of branches directly in the view method.
By moving this into the model we move a bunch of branching logic out of the view layer and make it easier to potentially reuse.
By moving this into the model we move a bunch of branching logic out of the view layer and make it easier to potentially reuse.
By moving this into the model we move a bunch of branching logic out of the view layer and make it easier to potentially reuse. Displayed postage is what we use to decide what to show in the corner of the envelope. If the letter can’t be posted (because validation failed) we don’t want to show any postage, becuase the letter will not be sent. Other parts of the code still need access to `.postage` to calculate delivery times etc.
By moving this into the model we can avoid defining another temporary variable that is just used to move data from the model to the template.
This is just bit nicer to read than digging into the JSON of a template.
A lot of the code in the view layer is now just passing data from the model to the template. If we give the template direct access to the model then we can: - remove some lines of code - remove a layer of indirection that needs following when reading the code
This variable was just passing data from the model to the template. The template has direct access to the model now so we can: - make the template refer to this data directly - encapsulate it into a method because it’s used more than once
Yet another bit of branching we can move out of the view layer
This function is now straightforward enough that it passes the complexity check.
The template always has access to `current_service` so there’s no need to pass it through from the view layer.
This wasn’t used anywhere in the template. In fact it was never used even when it was added 7 years ago: c5f92ea#diff-a44631c6318085fe6e2da66059480f9d62cd1d0aa75f41c8802f2fdb95b2c9c5R50
The code that uses this object gets the reply to text from the notification itself, not the template. So I don’t think this overriding is used.
quis
force-pushed
the
refactor-single-notification-into-model
branch
from
2d0f5bb to
7ed9b48
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This continues the work done in #5232
Notifications are one of the last database objects in the admin app that we are still passing around as raw JSON, rather than wrapping them in a proper Python object.