Skip to content

If a solution exists but we want to remove it it should be deleted from the remote#200

Merged
ColdHeat merged 1 commit intomasterfrom
delete-existing-solution
Feb 26, 2026
Merged

If a solution exists but we want to remove it it should be deleted from the remote#200
ColdHeat merged 1 commit intomasterfrom
delete-existing-solution

Conversation

@ColdHeat
Copy link
Member

  • Handle situation where an admin wants to remove solutions

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds functionality to delete remote solutions when they are removed locally, addressing a gap where previously, removing a solution configuration from the local challenge would leave the solution on the remote server.

Changes:

  • Adds logic to delete existing remote solutions when no local solution is defined or the solution file is missing
  • Implements the deletion check before attempting to create/update the solution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +922 to +924
resolved_solution = self._resolve_solution_path()
if not resolved_solution:
self._delete_existing_solution()
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The solution sync logic has inefficiencies and inconsistencies compared to other fields:

  1. Duplicate path resolution: _resolve_solution_path() is called twice - once here and once inside _create_solution(). This happens in both cases (solution exists or not).

  2. Pattern inconsistency: This doesn't follow the established pattern used for flags (lines 847-850), topics (lines 853-856), tags (lines 859-862), and hints (lines 907-910), which always delete then conditionally create.

Consider restructuring to match the established pattern:

  • Always call _delete_existing_solution()
  • Conditionally call _create_solution() (which already returns early if no solution)

This would eliminate the duplicate path resolution and improve code consistency.

Suggested change
resolved_solution = self._resolve_solution_path()
if not resolved_solution:
self._delete_existing_solution()
self._delete_existing_solution()

Copilot uses AI. Check for mistakes.
resolved_solution = self._resolve_solution_path()
if not resolved_solution:
self._delete_existing_solution()
self._create_solution()
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The new behavior to delete existing solutions when no local solution is defined lacks test coverage. Given that the TestSyncChallenge class has comprehensive tests for similar delete-on-removal behavior for other fields (flags, topics, tags, hints, files), a test should be added for solutions as well.

Consider adding a test that:

  1. Sets up a challenge with an existing remote solution
  2. Syncs with no local solution defined (or solution file missing)
  3. Verifies that the delete API call is made to remove the remote solution
Suggested change
self._create_solution()
else:
self._create_solution()

Copilot uses AI. Check for mistakes.
@ColdHeat ColdHeat merged commit 9055ae1 into master Feb 26, 2026
16 checks passed
@ColdHeat ColdHeat deleted the delete-existing-solution branch February 26, 2026 17:06
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.

2 participants