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.

Sign up for GitHub

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

Jump to bottom

delete repository inline #2396

Merged
merged 5 commits into from
Nov 16, 2024
Merged

delete repository inline #2396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aad2442
add repo id
rohitpaulk Nov 13, 2024
db04d32
Update delete repository modal to redirect to course introduction aft…
rohitpaulk Nov 13, 2024
ab37245
Add loading state for delete button and prevent multiple deletions in…
rohitpaulk Nov 13, 2024
580c4cd
Remove unused import of window from delete-repository-modal component.
rohitpaulk Nov 13, 2024
53b3f40
Update course card query to default to null for last used repository.
rohitpaulk Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/components/course-card.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export default class CourseCardComponent extends Component {
return {
name: 'course',
model: this.args.course.slug,
query: { fresh: null },
query: { repo: null }, // Route will pickup the last used repository by default
};
} else {
return {
Expand Down
6 changes: 5 additions & 1 deletion app/components/course-page/delete-repository-modal.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
</div>

<DangerButtonWithTimedConfirmation @onConfirm={{this.deleteRepository}} @size="regular" data-test-delete-repository-button>
Delete
{{#if this.isDeleting}}
Deleting...
{{else}}
Delete
{{/if}}
</DangerButtonWithTimedConfirmation>
</ModalBody>
16 changes: 12 additions & 4 deletions app/components/course-page/delete-repository-modal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
import RepositoryModel from 'codecrafters-frontend/models/repository';
import RouterService from '@ember/routing/router-service';
import Store from '@ember-data/store';
import window from 'ember-window-mock';
import { action } from '@ember/object';
import { inject as service } from '@ember/service';
import { tracked } from '@glimmer/tracking';

interface Signature {
Element: HTMLDivElement;
Expand All @@ -16,6 +16,8 @@
}

export default class DeleteRepositoryModalComponent extends Component<Signature> {
@tracked isDeleting: boolean = false;

@service declare router: RouterService;
@service declare store: Store;

Expand All @@ -25,9 +27,15 @@
return;
}

this.router.transitionTo('catalog');
await this.args.repository.destroyRecord();
window.location.reload();
if (this.isDeleting) {
return;
}

Check warning on line 32 in app/components/course-page/delete-repository-modal.ts

View check run for this annotation

Codecov / codecov/patch

app/components/course-page/delete-repository-modal.ts#L32 

Added line #L32 was not covered by tests

this.isDeleting = true;

Comment on lines +30 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Add test for concurrent deletion prevention

The test suite lacks coverage for the concurrent deletion prevention logic (isDeleting flag). The existing tests only verify basic deletion flow but don't test the protection against multiple deletion attempts. Add a test case that:

  • Triggers multiple delete clicks in quick succession
  • Verifies only one deletion request is made
  • Confirms the repository is deleted successfully

Example test:

test('prevents concurrent deletion attempts', async function (assert) {
  // ... setup code ...
  
  await coursePage.deleteRepositoryModal.deleteRepositoryButton.press();
  await coursePage.deleteRepositoryModal.deleteRepositoryButton.press(); // Second click should be ignored
  
  assert.strictEqual(server.schema.repositories.all().length, 0, 'repository should be deleted once');
});
🔗 Analysis chain

Add test coverage for concurrent deletion prevention

The concurrent deletion prevention logic is not covered by tests. This is important functionality that should be verified.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for existing tests of concurrent deletion prevention

# Look for test files related to delete repository
fd "delete-repository.*test" -t f

# Search for concurrent deletion tests in found files
rg -l "concurrent|multiple.*deletion|isDeleting" $(fd "delete-repository.*test" -t f)

Length of output: 233

Script:

#!/bin/bash
# Let's examine the actual test content to verify coverage
cat tests/acceptance/course-page/delete-repository-test.js

# Also check the component implementation for complete context
cat app/components/course-page/delete-repository-modal.ts

Length of output: 8523

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 32-32: app/components/course-page/delete-repository-modal.ts#L32
Added line #L32 was not covered by tests

const trackSlug = this.args.repository.language?.slug; // Store this before we destroy the record
await this.args.repository.destroyRecord(); // TODO: Add failure handling
Comment on lines +36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and null checks

Two concerns in the deletion logic:

  1. The TODO comment indicates missing error handling for destroyRecord()
  2. The trackSlug derivation doesn't handle the case where language is null

Consider implementing this safer version:

-    const trackSlug = this.args.repository.language?.slug; // Store this before we destroy the record
-    await this.args.repository.destroyRecord(); // TODO: Add failure handling
+    const trackSlug = this.args.repository.language?.slug || 'unknown';
+    try {
+      await this.args.repository.destroyRecord();
+    } catch (error) {
+      // Handle error (e.g., show error message to user)
+      this.isDeleting = false;
+      throw error;
+    }

Committable suggestion skipped: line range outside the PR's diff.

this.router.transitionTo('course.introduction', { queryParams: { repo: 'new', track: trackSlug } }).followRedirects();
}
}

Expand Down
5 changes: 2 additions & 3 deletions tests/acceptance/course-page/delete-repository-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,11 @@ module('Acceptance | course-page | delete-repository-test', function (hooks) {
assert.notOk(coursePage.deleteRepositoryModal.deleteRepositoryButton.progressIndicator.isVisible, 'progress indicator should not be visible');

await coursePage.deleteRepositoryModal.deleteRepositoryButton.press();
await waitUntil(() => currentURL() === '/catalog');
await waitUntil(() => currentURL() === '/courses/dummy/introduction?repo=new&track=python');
await settled(); // Delete request triggers after redirect

await catalogPage.clickOnCourse('Build your own Dummy');
await courseOverviewPage.clickOnStartCourse();
await coursePage.repositoryDropdown.click();
assert.strictEqual(coursePage.repositoryDropdown.content.nonActiveRepositoryCount, 0, 'no repositories should be available');
assert.notOk(coursePage.repositoryDropdown.content.text.includes('Delete Repository'), 'delete repository action should not be available');
});
});