Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

Fix delete note from detail view #954

Merged
merged 16 commits into from
Oct 24, 2023
Merged

Fix delete note from detail view #954

merged 16 commits into from
Oct 24, 2023

Conversation

gordonbrander
Copy link
Collaborator

@gordonbrander gordonbrander commented Oct 19, 2023 •
edited
Loading

Fixes #941

  • Fix Notebook tab
    • Fix deletion
    • Add tests
  • Fix Feed tab
    • Fix deletion
    • Add tests
  • Fix Profile tab
    • Fix deletion
    • Add tests

QA:

  • Notebook tab
    • Check delete
    • Check rename
    • Check change audience
  • Feed tab
    • Check delete
    • Check rename
    • Check change audience
  • Profile tab
    • Check delete
    • Check rename
    • Check change audience

@gordonbrander
Fix delete memo in notebook
82b58f9
@gordonbrander
Add tests for NotebookDetailStackCursor.tag
90d7091 
and make NotebookAction conform to Hashable so we can do equality tests.
This involved sending an error string, not an Error in the action, and
marking a type Hashable.
@gordonbrander gordonbrander changed the title 2023 10 19 fix delete in note Fix delete note from detail view Oct 19, 2023
gordonbrander
gordonbrander commented Oct 19, 2023
View reviewed changes
xcode/Subconscious/Shared/Components/Notebook/Notebook.swift
@@ -211,6 +211,8 @@ struct NotebookDetailStackCursor: CursorProtocol {

static func tag(_ action: ViewModel.Action) -> NotebookModel.Action {
switch action {
case let .requestDeleteMemo(slashlink):
return .requestDeleteMemo(slashlink)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add missing requestDelete interception. This fixes deletion in detail view.

@gordonbrander
Fix indenting that xcode borked behind my back
6a18450
@gordonbrander
Continue wiring up lifecycle actions for delete
4ffcfa5 
...on profile
@gordonbrander
Animate profile changes
d7dec9f
@gordonbrander
Remove refreshall from HomeProfileView
260fdd4 
Not needed in this case because the view refreshes itself on appear.
@gordonbrander
Add tests for Notebook action functions
d57d0bb 
Also make AppAction Hashable so we can compare it in tests.
@gordonbrander
Add feed tests
b3f5349 
...and make FeedAction Hashable for testing.
@gordonbrander
Add tests for HomeProfileActions
5432daf
@gordonbrander
Copy link
Collaborator Author

Note to self: the current information flow goes:

  • Editor sends up requestDelete() notification.
  • Up to DetailStackAction.requestDelete()
  • Up to NotebookAction.requestDelete()
  • Intercepted and replayed on AppStore as AppAction.deleteMemo()
  • Memo is deleted
  • Tabs intercept and replay .succeedDeleteMemo(), etc.

@gordonbrander gordonbrander marked this pull request as ready for review October 23, 2023 16:32
@gordonbrander
Copy link
Collaborator Author

@bfollington this PR is ready to go, but an unrelated test Tests_DataService.testConcurrentIndexing() is causing the PR to fail.

I think that Tests_DataService.testConcurrentIndexing may be saturating the testrunner with dummy tasks. Can you say more about this test, what it is accomplishing, and how we might go about fixing this?

bfollington
bfollington approved these changes Oct 24, 2023
View reviewed changes
Copy link
Collaborator

@bfollington bfollington left a comment

Choose a reason for hiding this comment

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

A couple of formatting suggestions, but LGTM.

One thought: do we need Hashable or just Equatable for tests? I don't have strong feelings but I believe Hashable extends Equatable so it's a (slightly) stronger contract.

@bfollington bfollington merged commit 36e4552 into main Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bfollington bfollington bfollington approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deleting from note detail does not work
2 participants
@gordonbrander @bfollington