Skip to content
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

Add command to repair notifications and properly handle mail sending … #333

Merged
merged 7 commits into from
Apr 15, 2021

Conversation

JammingBen
Copy link
Contributor

…with relative urls

@CLAassistant
Copy link

CLAassistant commented Apr 14, 2021

CLA assistant check
All committers have signed the CLA.

lib/Command/RepairNotifications.php Outdated Show resolved Hide resolved
lib/Command/RepairNotifications.php Outdated Show resolved Hide resolved
lib/Command/RepairNotifications.php Outdated Show resolved Hide resolved
lib/Command/RepairNotifications.php Outdated Show resolved Hide resolved
lib/Command/RepairNotifications.php Outdated Show resolved Hide resolved
@jvillafanez
Copy link
Member

Something to consider is to apply directly the "update" without the "select". I'm not sure if it's possible.
I don't think that a "repairing notification ID 23" message is good enough. You still need to check the DB to have information about what is the notification you're talking about. Assuming you can update all the notifications in bulk, you reduce the DB load: 1 update vs 1 select + n updates. You can show an "updated 56 notifications" afterwards (the update query should return that value)

@JammingBen
Copy link
Contributor Author

@jvillafanez Included your suggestions, please have a look again.

Something to consider is to apply directly the "update" without the "select". I'm not sure if it's possible.

Tried this, unfortunately it's not that easy as we would need to implement some kind of regex mechanism. This could work for the link column, but the fact we also have to update the actions column (which stores an array of "objects") makes it even harder. IMO the load on the database it OK as this is nothing which runs on a regular basis.

@JammingBen JammingBen requested a review from jvillafanez April 14, 2021 12:44
Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

tiny details

lib/Handler.php Outdated Show resolved Hide resolved
lib/Handler.php Outdated Show resolved Hide resolved
@JammingBen JammingBen requested a review from jvillafanez April 15, 2021 06:58
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AlexAndBear
Copy link

@AlexAndBear AlexAndBear merged commit dac2400 into master Apr 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix-absolute-urls branch April 15, 2021 08:11
@jvillafanez
Copy link
Member

Probably, but I'd let the docs team decide.

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.

4 participants