Skip to content

feat: add delete button to question and news forms#4643

Open
GMetaxakis wants to merge 13 commits intoONEARMY:masterfrom
GMetaxakis:feat/delete-questions-news
Open

feat: add delete button to question and news forms#4643
GMetaxakis wants to merge 13 commits intoONEARMY:masterfrom
GMetaxakis:feat/delete-questions-news

Conversation

@GMetaxakis
Copy link
Copy Markdown
Contributor

@GMetaxakis GMetaxakis commented Mar 1, 2026

PR Checklist

  • - Unit and/or e2e tests for the changes that have been added (for bug fixes / features)

PR Type

  • Feature

What is the new behavior?

  • Add DELETE API endpoints to api.questions.$id and api.news.$id with creator/admin authorization (soft-delete via deleted: true)
  • Add delete button with confirmation modal to QuestionForm and NewsForm (visible only in edit mode), matching the existing ResearchUpdateForm pattern
  • Add client-side deleteQuestion and deleteNews service functions
  • Add Cypress e2e tests for deleting questions and news

Does this PR introduce a DB Schema Change or Migration?

  • No

Git Issues

Closes #4265

Add soft-delete API endpoints (DELETE method) to api.questions.$id and
api.news.$id with creator/admin authorization. Add client-side delete
functions, delete buttons with confirmation modals in the edit forms,
and Cypress tests for both content types.

Closes ONEARMY#4265
@GMetaxakis GMetaxakis force-pushed the feat/delete-questions-news branch from 3748fb6 to 41ba2ec Compare March 1, 2026 15:11
@cypress
Copy link
Copy Markdown

cypress bot commented Mar 1, 2026

onearmy-community-platform    Run #9133

Run Properties:  status check failed Failed #9133  •  git commit 910b9d4955: Merge branch 'feat/delete-questions-news' of https://github.com/GMetaxakis/commu...
Project onearmy-community-platform
Branch Review pull/4643
Run status status check failed Failed #9133
Run duration 30m 26s
Commit git commit 910b9d4955: Merge branch 'feat/delete-questions-news' of https://github.com/GMetaxakis/commu...
Committer Georgios Metaxakis
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 96
View all changes introduced in this branch ↗︎

Tests for review

Failed  src/integration/news/write.spec.ts • 1 failed test • ci-chrome

View Output Video

Test Artifacts
[News.Write] > [Delete a news item] > [By Author] Test Replay Screenshots Video

@GMetaxakis
Copy link
Copy Markdown
Contributor Author

The E2E test failures are caused by the new published_at column not existing on the CI test Supabase instance.

The migration (20260301100000_add_news_published_at.sql) needs to be applied to the test database for the news API queries to work:

ALTER TABLE news ADD COLUMN published_at timestamptz;
UPDATE news SET published_at = created_at WHERE (is_draft = false OR is_draft IS NULL);

Once the migration is applied, the 4 failing news tests should pass.

@GMetaxakis GMetaxakis changed the title feat: add delete button to question and news forms fix: show publish date instead of draft creation date for news Mar 1, 2026
@benfurber
Copy link
Copy Markdown
Member

Hey @GMetaxakis, thanks for working on so many items atm. I'm confused by the branch name and some some of the changes in this branch. Are they all related to the creation date change?

@GMetaxakis GMetaxakis force-pushed the feat/delete-questions-news branch from d5ea96a to 41ba2ec Compare March 5, 2026 16:26
@GMetaxakis GMetaxakis changed the title fix: show publish date instead of draft creation date for news feat: add delete button to question and news forms Mar 5, 2026
@GMetaxakis
Copy link
Copy Markdown
Contributor Author

Hey @benfurber, you're right — sorry about the confusion! I accidentally pushed the publish date changes onto this branch instead of keeping them on their own. I've now cleaned this up:

Should be much clearer now. Let me know if you have any questions!

@mariojsnunes
Copy link
Copy Markdown
Contributor

@GMetaxakis seems like both cypress tests for deleting news and questions are failing

The delete tests for questions and news used [data-cy=confirm] but the
ConfirmModal component uses data-cy="Confirm.modal: Confirm".
@mariojsnunes mariojsnunes added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Mar 18, 2026
@mariojsnunes mariojsnunes added Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview and removed Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview labels Mar 23, 2026
@mariojsnunes
Copy link
Copy Markdown
Contributor

Fixed the conflicts on this branch

@mariojsnunes
Copy link
Copy Markdown
Contributor

@dalibormrska need your input here.
we have 2 ways of deleting an item currently.

For research appears on the item itself.
image

With this PR questions and library will have it on the form (need to edit first):
image

Technically it's simpler to not have it on the form, but we can do it however is best for UX.

@dalibormrska
Copy link
Copy Markdown
Member

Judging now the logic decision and not necessarily the current layout, I lean towards the option of having just the edit button and the delete inside of the form.

Copy link
Copy Markdown
Contributor

@mariojsnunes mariojsnunes left a comment

Choose a reason for hiding this comment

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

just a minor thing about error handling.

we are now sending the error messages correctly from the api. so we should handle them correctly on the frontend as well.

Also please ensure my conflict resolution didn't break something :)

});

if (response.status !== 200 && response.status !== 201) {
throw new Error('Error deleting news', { cause: 500 });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should handle the error with the message sent from the api, see above.

});

if (response.status !== 200 && response.status !== 201) {
throw new Error('Error deleting question', { cause: 500 });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(same as on newsService)
Should handle the error with the message sent from the api, see above.

return;
}
setShowDeleteModal(false);
await newsService.deleteNews(id);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should handle the error here, maybe show an <Alert /> component

return;
}
setShowDeleteModal(false);
await questionService.deleteQuestion(id);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should handle the error here, maybe show an <Alert /> component

- Read error messages from API response in newsService and
  questionService delete functions instead of using generic messages
- Catch delete errors in NewsForm and QuestionForm and display them
  via the existing saveErrorMessage/FormWrapper error UI
@mariojsnunes
Copy link
Copy Markdown
Contributor

@GMetaxakis the news delete test is failing

The news listing query was missing the deleted filter, causing
soft-deleted news items to still appear in the /news page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview

Projects

Status: No status
Status: In progress

Development

Successfully merging this pull request may close these issues.

[feature] Delete button for all forms

4 participants