Saleor 1906 user should be able to delete a channel if there are no orders associated with it #901

MarekChoinski · 2020-12-14T10:50:54Z

I want to merge this change because... it

fixes bug with deleting channels when hasOrders was false
adds "DELETE" button on detail page of channel

PR intended to be tested with API branch:

Screenshots

Pull Request Checklist

All visible strings are translated with proper context.
All data-formatting is locale-aware (dates, numbers, and so on).
Translated strings are extracted.
Number of API calls is optimized.
The changes are tested.
Data-test are added for new elements.
Type definitions are up to date.
Changes are mentioned in the changelog.

Test environment config

API_URI=https://master.staging.saleor.rocks/graphql/

krzysztofwolski

One change, besides that, LGTM

krzysztofwolski · 2020-12-14T15:18:14Z

src/channels/components/ChannelDeleteDialog/ChannelDeleteDialog.tsx

@@ -42,52 +44,56 @@ const ChannelDeleteDialog: React.FC<ChannelDeleteDialogProps> = ({
      confirmButtonState={confirmButtonState}
      open={open}
      onClose={onClose}
-      onConfirm={() => (hasChannels ? onConfirm(choice) : onBack())}
+      onConfirm={() =>
+        hasChannels || !hasOrders ? onConfirm(choice) : onBack()


It seems to be reused multiple times. How about :
const canBeDeleted = hasChannels || !hasOrders

src/channels/index.tsx

bmigirl · 2020-12-16T09:54:18Z

src/channels/components/ChannelDeleteDialog/ChannelDeleteDialog.tsx

+            <>
+              <Typography>
+                <FormattedMessage
+                  defaultMessage="All order information from this channel need to be moved to a different channel. Please select channel orders need to be moved to:."


There are quite some long messages in this component, would rather want to have them in defineMessages somewhere above, so they don't stand out so much (especially considering the line length) in the component code itself

bmigirl · 2020-12-16T09:58:43Z

src/channels/views/ChannelDetails/ChannelDetails.tsx

+    onCompleted
+  });
+
+  const channelsChoices = id


Seems close enough copy pasted from src/channels/views/ChannelsList/ChannelsList.tsx - can we make a utils function (like getParsedChannelsChoices or something similar) out of this?

bmigirl · 2020-12-16T10:04:00Z

src/channels/views/ChannelDetails/ChannelDetails.tsx

+    : [];
+
+  const handleRemoveConfirm = (targetChannelId?: string) => {
+    if (targetChannelId) {


Don't think we need and if/else here, since the same action happens in both cases, maybe we can simplify:

Suggested change

if (targetChannelId) {

const data = targetChannelId

? { id, input: { targetChannel: targetChannelId } }

: { id }

deleteChannel({ variables: data })

bmigirl · 2020-12-16T10:05:02Z

src/channels/views/ChannelsList/ChannelsList.tsx

-    deleteChannel({
-      variables: { id: params.id, input: { targetChannel: id } }
-    });
+  const handleRemoveConfirm = (targetChannelId?: string) => {


Same as above

src/channels/utils.ts

github-actions bot temporarily deployed to saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 14, 2020 10:51 Inactive

github-actions bot temporarily deployed to storybook saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 14, 2020 10:51 Inactive

github-actions bot temporarily deployed to saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 14, 2020 11:01 Inactive

github-actions bot temporarily deployed to storybook saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 14, 2020 11:01 Inactive

krzysztofwolski requested changes Dec 14, 2020

View reviewed changes

krzysztofwolski requested review from dominik-zeglen, orzechdev and bmigirl December 14, 2020 15:21

github-actions bot temporarily deployed to saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 14, 2020 17:00 Inactive

github-actions bot temporarily deployed to storybook saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 14, 2020 17:00 Inactive

jwm0 suggested changes Dec 16, 2020

View reviewed changes

src/channels/index.tsx Outdated Show resolved Hide resolved

bmigirl suggested changes Dec 16, 2020

View reviewed changes

github-actions bot temporarily deployed to saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 16, 2020 10:11 Inactive

github-actions bot temporarily deployed to storybook saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 16, 2020 10:11 Inactive

github-actions bot temporarily deployed to saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 16, 2020 14:47 Inactive

github-actions bot temporarily deployed to storybook saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 16, 2020 14:47 Inactive

github-actions bot temporarily deployed to saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 16, 2020 14:56 Inactive

github-actions bot temporarily deployed to storybook saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar December 16, 2020 14:56 Inactive

krzysztofwolski approved these changes Dec 17, 2020

View reviewed changes

MarekChoinski requested review from jwm0 and bmigirl December 17, 2020 09:56

orzechdev approved these changes Dec 17, 2020

View reviewed changes

bmigirl approved these changes Jan 7, 2021

View reviewed changes

dominik-zeglen changed the title ~~Saleor 1906 user should be able to create a channel if there are no orders associated with it~~ Saleor 1906 user should be able to delete a channel if there are no orders associated with it Jan 12, 2021

dominik-zeglen approved these changes Jan 12, 2021

View reviewed changes

src/channels/utils.ts Outdated Show resolved Hide resolved

MarekChoinski added 5 commits January 12, 2021 13:26

Update type of channelDetailsFragment

b40815e

Add hasOrder recognition

e584a81

Update types

bfe9d26

Add delete channel on details page

5136032

Refactor fixtures

0c4caf2

MarekChoinski added 9 commits January 12, 2021 13:26

Update locale

2bc0578

Refactor

7575d5b

Remove redundant assignment

6098ea8

Move messages to defineMessages

493e1ec

Simplify variables

a723c9e

Create util getChannelsCurrencyChoices

8687f24

Add message

5ffb306

Update locale

95df27a

Refactor to mapNodeToChoice

4f58c2e

MarekChoinski force-pushed the SALEOR-1906-user-should-be-able-to-create-a-channel-if-there-are-no-orders-associated-with-it branch from 5e4db93 to 4f58c2e Compare January 12, 2021 12:44

github-actions bot temporarily deployed to saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar January 12, 2021 12:44 Inactive

github-actions bot deployed to storybook saleor-1906-user-should-be-able-to-create-a-channel-if-there-ar January 12, 2021 12:44 View deployment

jwm0 approved these changes Jan 12, 2021

View reviewed changes

jwm0 merged commit 3f5cacb into master Jan 12, 2021

jwm0 deleted the SALEOR-1906-user-should-be-able-to-create-a-channel-if-there-are-no-orders-associated-with-it branch January 12, 2021 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Saleor 1906 user should be able to delete a channel if there are no orders associated with it #901

Saleor 1906 user should be able to delete a channel if there are no orders associated with it #901

MarekChoinski commented Dec 14, 2020

krzysztofwolski left a comment

krzysztofwolski Dec 14, 2020

bmigirl Dec 16, 2020

bmigirl Dec 16, 2020

bmigirl Dec 16, 2020

bmigirl Dec 16, 2020

-    if (targetChannelId) {
+ const data = targetChannelId
+    ? { id, input: { targetChannel: targetChannelId } }
+    : { id }
+ deleteChannel({ variables: data })

Saleor 1906 user should be able to delete a channel if there are no orders associated with it #901

Saleor 1906 user should be able to delete a channel if there are no orders associated with it #901

Conversation

MarekChoinski commented Dec 14, 2020

Screenshots

Pull Request Checklist

Test environment config

krzysztofwolski left a comment

Choose a reason for hiding this comment

krzysztofwolski Dec 14, 2020

Choose a reason for hiding this comment

bmigirl Dec 16, 2020

Choose a reason for hiding this comment

bmigirl Dec 16, 2020

Choose a reason for hiding this comment

bmigirl Dec 16, 2020

Choose a reason for hiding this comment

bmigirl Dec 16, 2020

Choose a reason for hiding this comment