Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

Level Deletion Confirmation and Bug Fixes #512

Merged
merged 2 commits into from Jun 21, 2017
Merged

Level Deletion Confirmation and Bug Fixes #512

merged 2 commits into from Jun 21, 2017

Conversation

justinwray
Copy link
Contributor

  • Added deletion confirmation dialog when deleting levels.

  • Level data is now deleted from HintLog, ScoresLog, and FailuresLog, when a level is deleted.

  • Cache invalidation performed on level deletion for records containing levels.

@justinwray
Copy link
Contributor Author

This commit includes the changes found in PR #509. Once #509 has been merged, this PR can be rebased.

* Added deletion confirmation dialog when deleting levels.

* Level data is now deleted from HintLog, ScoresLog, and FailuresLog, when a level is deleted.

* Cache invalidation performed on level deletion for records containing levels.
@justinwray
Copy link
Contributor Author

This PR is now mergeable.

Given rebase conflicts: the branch was reset (hard) to the upstream dev branch, and then the PR changes were patched and merged onto dev, submitted via a (force) push. All relevant commit log data has been retained in the final commit message.

await $db->queryf('DELETE FROM levels WHERE id = %d LIMIT 1', $level_id);
await $db->queryf('DELETE FROM hints_log WHERE level_id = %d', $level_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to try and combine awaits together with something like \HH\Asio\va

You can do that for loops too by putting the Awaitables in an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved all awaitable queries into a vector which are now executed via AsyncMysqlConnection::multiQuery().

@justinwray
Copy link
Contributor Author

Updated based on feedback.

@gsingh93
Copy link
Contributor

The use of sprintf here is fine because we're replacing id, but just remember in the general case it's not fine and will lead to SQL injection. When dealing with multiple unsafe queries we need to collect all the Awaitables from queryf in an array, and then use \HH\Asio\v or a similar function to convert the array of Awaitables into a single Awaitable, and then Await on that.

But since these queries only use id, which is not controlled by the user, I'll go ahead an accept this.

@gsingh93 gsingh93 merged commit 4a7b5b5 into facebookarchive:dev Jun 21, 2017
@justinwray justinwray mentioned this pull request Aug 5, 2017
justinwray added a commit that referenced this pull request Aug 5, 2017
Merge of `dev` into `master`

Commits:

* Registration enforcing strong passwords (#442) (ac64f55)
* Custom branding for icon and text (#448) (081062c)
* Merge of /master into /dev - Baseline for Development (#509) (25c1748)
* Updated Language Translations (#511) (b9f031e)
* Auto Announcements and Activity Log Expansion (#513) (323ba05)
* Level Import Fix (#514) (dc7c87c)
* Announcements Controls Rename (#515) (c5da9f7)
* Set Default Scoring Cache Values (#516) (ec996a5)
* Unique Logos Per Team # (#517) (6d4f919)
* Custom Branding Update (#518) (ea78f6a)
* Backup and Restore settings.ini on Tests (#519) (eb4a5b5)
* Maintain Team Protection on Database Reset (#520) (5d91ae9)
* Fixed Login Form JS Bug (Fixes: #521) (#523) (2b1474b)
* Level Deletion Confirmation and Bug Fixes (#512) (4a7b5b5)
* Provision Streamlined, Quick Setup Added, and Multiple Containers Support (#535) (b487fc1)
* Merge branch 'dev' into WraySec/fbctf/merge@7f8c281
iliushin-a added a commit to iliushin-a/fbctf that referenced this pull request May 16, 2018
* Level Deletion Confirmation and Bug Fixes

* Added deletion confirmation dialog when deleting levels.

* Level data is now deleted from HintLog, ScoresLog, and FailuresLog, when a level is deleted.

* Cache invalidation performed on level deletion for records containing levels.

* Moved all awaitable queries into a vector which are now executed via AsyncMysqlConnection::multiQuery().
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants