Page MenuHomePhabricator

Implement draft reducer and use it in master reducer
ClosedPublic

Authored by marcin on Nov 16 2022, 5:07 AM.
Tags
None
Referenced Files
F3360647: D5646.diff
Sun, Nov 24, 1:46 PM
F3360035: D5646.id.diff
Sun, Nov 24, 11:52 AM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Subscribers

Details

Summary

This differential implements draft reducer and uses it in master reducer

Test Plan

Make sure eslint and flow do not point errors.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/types/store-ops-types.js
1 ↗(On Diff #18471)

Add newline

atul requested changes to this revision.Nov 17 2022, 12:24 PM

Can you explain why draftStoreOperations is empty for the setDraftStoreDrafts action?

I may be missing something... but at least based on my understand of the way messageStoreOperations and threadStoreOperations work this seems like it might be wrong?

lib/reducers/draft-reducer.js
20–21 ↗(On Diff #18529)

Whatever you prefer here, but can make a little more concise

23–28 ↗(On Diff #18529)

Whatever you prefer here, but can make a little more concise

50 ↗(On Diff #18529)

This is pretty smooth, took me a second lol

67 ↗(On Diff #18529)

Wait are you sure draftStoreOperations should be empty in this case?

I'd assume we'd want an update op for each entry in drafts to populate the SQLite drafts table?

Probably a remove_all sort of op followed by a list of update ops?

This revision now requires changes to proceed.Nov 17 2022, 12:24 PM

Wait no completely disregard what I said, setDraftStoreDrafts is equivalent to SET_MESSAGE_STORE_MESSAGES or SET_THREAD_STORE so shouldn't have any draftStoreOperations... we're just populating the Redux store here... sorry about that confusion.

This revision is now accepted and ready to land.Nov 17 2022, 12:25 PM

Unpack action payload more concisely

tomek requested changes to this revision.Nov 18 2022, 3:40 AM
tomek added inline comments.
lib/reducers/draft-reducer.js
19 ↗(On Diff #18557)

We probably need one more if to delete the drafts when logging out - mostly for persistence on web

44 ↗(On Diff #18557)

You can use a shorthand

68 ↗(On Diff #18557)
This revision now requires changes to proceed.Nov 18 2022, 3:40 AM
lib/reducers/draft-reducer.js
19 ↗(On Diff #18557)

Please refer to the old draft reducer I linked to you previously

React to log out action in draft-reducer

lib/reducers/draft-reducer.js
30 ↗(On Diff #18582)

You aren't doing anything to clear out the drafts from SQLite when the user logs out. I know the database will be deleted, but we probably want to do the same thing we do for threads and messages... when the user logs out, have a operation that clears out the contents

ashoat requested changes to this revision.Nov 18 2022, 12:39 PM
This revision now requires changes to proceed.Nov 18 2022, 12:39 PM

Add remove all drafts operations to draft reducer

This revision is now accepted and ready to land.Nov 21 2022, 4:38 AM

Spacing nits

lib/reducers/draft-reducer.js
15

Add newline above here

18

Add newline below here