-
Notifications
You must be signed in to change notification settings - Fork 22
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
Bump noticed from 1.6.3 to 2.3.2 #6889
Conversation
505261d
to
86d0619
Compare
ee6b78d
to
1a14633
Compare
OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting If you change your mind, just re-open this PR and I'll resolve any conflicts on it. |
1a14633
to
86d0619
Compare
Bumps [noticed](https://github.com/excid3/noticed) from 1.6.3 to 2.3.2. - [Release notes](https://github.com/excid3/noticed/releases) - [Changelog](https://github.com/excid3/noticed/blob/main/CHANGELOG.md) - [Upgrade guide](https://github.com/excid3/noticed/blob/main/UPGRADE.md) - [Commits](excid3/noticed@v1.6.3...v2.3.2) --- updated-dependencies: - dependency-name: noticed dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…e in their upgrade docs
5a379c2
to
8072293
Compare
…del anymore so cannot use the previous syntax
3f5cb6f
to
05930d5
Compare
Due to the changes in the gem upgrade the time travel blocks do not work for out purpose in these tests. I believe this is because the noticed event is created inside the time travel block, but that creates noticed notifications in the background outside of the time travel block, so i have stubbed the created_at on the notifications to recreate these tests as best I can.
These specs only test the timestamp method, and are very flaky due to the way the upgrade to the noticed gem works. Now the event is created when the deliver method is run and in turn this created notifications in the background, making these tests are flaky. I think this in combination with how little value these tests add make removing them a sensible idea.
@@ -12,11 +12,11 @@ def index | |||
|
|||
def notifications | |||
@notifications ||= current_publisher.notifications | |||
.created_within_data_access_period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This scope is on the old notifications model, which will be deleted. It is only referenced here.
.order(created_at: :desc) | ||
end | ||
|
||
def mark_notifications_as_read | ||
notifications.mark_as_read! | ||
notifications.mark_as_read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is because in the new version of noticed mark_as_read! has been renamed to mark_as_read
@@ -1,4 +1,4 @@ | |||
class SendJobApplicationDataExpiryNotificationJob < ApplicationJob | |||
class SendJobApplicationDataExpiryNotifierJob < ApplicationJob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In noticed v2 notifiers are the class that delivers notifications Notification - the database record of the notification. They recommend changing the classes to match this.
@@ -3,8 +3,6 @@ class ApplicationRecord < ActiveRecord::Base | |||
|
|||
before_save :strip_attributes | |||
|
|||
DATA_ACCESS_PERIOD_FOR_PUBLISHERS = 1.year.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DATA_ACCESS_PERIOD_FOR_PUBLISHERS needs to be available in both controllers and models now so I have added it to a data_access_period initializer.
@@ -0,0 +1 @@ | |||
DATA_ACCESS_PERIOD_FOR_PUBLISHERS = 1.year.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DATA_ACCESS_PERIOD_FOR_PUBLISHERS needs to be available in both controllers and models now so I have added it to a data_access_period initializer.
@@ -1,8 +1,7 @@ | |||
class Notification < ApplicationRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class needs to remain for now so that we can run the migration to migrate our previous notifications over to the new noticed_notifications table. I have had to leave some validations in this for now for our database consistency checks. Once the migration is complete we can delete the notifications table and this model.
@@ -15,7 +15,7 @@ | |||
.govuk-body.float-right | |||
= pagy_stats(@pagy, type: "notification") | |||
|
|||
= render @notifications | |||
= render partial: "notification", collection: @notifications, as: :notification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to notifications now being called noticed_notifications we need to explicitly set this collection partial.
event = Noticed::Event.find(notification.event_id) | ||
|
||
expect(notification).to have_attributes(expected_notification_attributes) | ||
expect(event).to have_attributes(expected_event_attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have two models/tables rather than just one and they store different attributes
delegate :created_at, to: :record | ||
param :vacancy, :publisher | ||
|
||
notification_methods do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of v2 we need to wrap these methods in the notifications_method block to make them available on the notification, rather than the event of the same name
delegate :created_at, to: :record | ||
param :vacancy, :job_application | ||
|
||
notification_methods do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of v2 we need to wrap these methods in the notifications_method block to make them available on the notification, rather than the event of the same name
Review app https://teaching-vacancies-review-pr-6889.test.teacherservices.cloud was successfully deleted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for the very helpful annotations to review the PR 👏
Bumps noticed from 1.6.3 to 2.3.2.
Release notes
Sourced from noticed's releases.
... (truncated)
Changelog
Sourced from noticed's changelog.
... (truncated)
Upgrade guide
Sourced from noticed's upgrade guide.
... (truncated)
Commits
0430be6
Version bump6dfb7d9
Add sqlserver support (#451)f52e870
Version bumpbc93b64
Skip application notifier if it already exists5a1fb91
Use matching sqlite3 versions for Rails versions8e2dc61
Merge branch 'main' of github.com:excid3/noticed68c406f
Version bump7cb10d1
Update twilio_messaging.md2ef3c18
Twilio delivery method allows Notifier to handle errors (#444)2a3dbb0
Don't remove record from params with ephemeral notifications (#448)Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditions
will show all of the ignore conditions of the specified dependency@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)