Browse Source

fix(settings): Sort newsletter subscriptions by itemId (#49968)

- Prevent them from re-arranging on click
- Fixes a width issue on smaller screens
- Makes the switches properly labeled

Still more questions about how multiple email addresses work on the
subscription page. We only seem to update the primary email and do not
pass the email address in the PUT request.
Scott Cooper 1 year ago
parent
commit
471b6d36f5

+ 38 - 12
static/app/views/settings/account/accountSubscriptions.spec.jsx

@@ -1,17 +1,16 @@
 import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
 
-import {Client} from 'sentry/api';
 import AccountSubscriptions from 'sentry/views/settings/account/accountSubscriptions';
 
 const ENDPOINT = '/users/me/subscriptions/';
 
 describe('AccountSubscriptions', function () {
   beforeEach(function () {
-    Client.clearMockResponses();
+    MockApiClient.clearMockResponses();
   });
 
   it('renders empty', function () {
-    Client.addMockResponse({
+    MockApiClient.addMockResponse({
       url: ENDPOINT,
       body: [],
     });
@@ -23,24 +22,21 @@ describe('AccountSubscriptions', function () {
   });
 
   it('renders list and can toggle', async function () {
-    Client.addMockResponse({
+    MockApiClient.addMockResponse({
       url: ENDPOINT,
       body: TestStubs.Subscriptions(),
     });
-    const mock = Client.addMockResponse({
+    const mock = MockApiClient.addMockResponse({
       url: ENDPOINT,
       method: 'PUT',
     });
-
-    const wrapper = render(<AccountSubscriptions />, {
-      context: TestStubs.routerContext(),
-    });
-
-    expect(wrapper.container).toSnapshot();
+    render(<AccountSubscriptions />);
 
     expect(mock).not.toHaveBeenCalled();
 
-    await userEvent.click(screen.getAllByTestId('switch')[0]);
+    await userEvent.click(
+      screen.getByRole('checkbox', {name: 'Product & Feature Updates'})
+    );
 
     expect(mock).toHaveBeenCalledWith(
       ENDPOINT,
@@ -53,4 +49,34 @@ describe('AccountSubscriptions', function () {
       })
     );
   });
+
+  it('can handle multiple email addresses', async function () {
+    MockApiClient.addMockResponse({
+      url: ENDPOINT,
+      body: [
+        ...TestStubs.Subscriptions().map(x => ({...x, email: 'a@1.com'})),
+        ...TestStubs.Subscriptions().map(x => ({...x, email: 'b@2.com'})),
+      ],
+    });
+    const mock = MockApiClient.addMockResponse({
+      url: ENDPOINT,
+      method: 'PUT',
+    });
+    render(<AccountSubscriptions />);
+
+    await userEvent.click(
+      screen.getAllByRole('checkbox', {name: 'Sentry Newsletter'})[0]
+    );
+
+    expect(mock).toHaveBeenCalledWith(
+      ENDPOINT,
+      expect.objectContaining({
+        method: 'PUT',
+        data: {
+          listId: 1,
+          subscribed: true,
+        },
+      })
+    );
+  });
 });

+ 65 - 43
static/app/views/settings/account/accountSubscriptions.tsx

@@ -41,13 +41,20 @@ class AccountSubscriptions extends AsyncView<AsyncView['props'], State> {
     return t('Subscriptions');
   }
 
-  handleToggle = (subscription: Subscription, listId: number, _e: React.MouseEvent) => {
+  handleToggle = (
+    subscription: Subscription,
+    email: string,
+    listId: number,
+    _e: React.MouseEvent
+  ) => {
     const subscribed = !subscription.subscribed;
     const oldSubscriptions = this.state.subscriptions;
 
     this.setState(state => {
-      const newSubscriptions = state.subscriptions.slice();
-      const index = newSubscriptions.findIndex(sub => sub.listId === listId);
+      const newSubscriptions = [...state.subscriptions];
+      const index = newSubscriptions.findIndex(
+        sub => sub.listId === listId && sub.email === email
+      );
       newSubscriptions[index] = {
         ...subscription,
         subscribed,
@@ -115,45 +122,52 @@ class AccountSubscriptions extends AsyncView<AsyncView['props'], State> {
                       </Heading>
                     )}
 
-                    {subscriptions.map(subscription => (
-                      <PanelItem center key={subscription.listId}>
-                        <SubscriptionDetails>
-                          <SubscriptionName>{subscription.listName}</SubscriptionName>
-                          {subscription.listDescription && (
-                            <Description>{subscription.listDescription}</Description>
-                          )}
-                          {subscription.subscribed ? (
-                            <SubscribedDescription>
-                              <div>
-                                {tct('[email] on [date]', {
-                                  email: subscription.email,
-                                  date: (
-                                    <DateTime
-                                      date={moment(subscription.subscribedDate!)}
-                                    />
-                                  ),
-                                })}
-                              </div>
-                            </SubscribedDescription>
-                          ) : (
-                            <SubscribedDescription>
-                              {t('Not currently subscribed')}
-                            </SubscribedDescription>
-                          )}
-                        </SubscriptionDetails>
-                        <div>
-                          <Switch
-                            isActive={subscription.subscribed}
-                            size="lg"
-                            toggle={this.handleToggle.bind(
-                              this,
-                              subscription,
-                              subscription.listId
+                    {subscriptions
+                      .sort((a, b) => a.listId - b.listId)
+                      .map(subscription => (
+                        <PanelItem center key={subscription.listId}>
+                          <SubscriptionDetails
+                            htmlFor={`${subscription.email}-${subscription.listId}`}
+                            aria-label={subscription.listName}
+                          >
+                            <SubscriptionName>{subscription.listName}</SubscriptionName>
+                            {subscription.listDescription && (
+                              <Description>{subscription.listDescription}</Description>
                             )}
-                          />
-                        </div>
-                      </PanelItem>
-                    ))}
+                            {subscription.subscribed ? (
+                              <SubscribedDescription>
+                                <div>
+                                  {tct('[email] on [date]', {
+                                    email: subscription.email,
+                                    date: (
+                                      <DateTime
+                                        date={moment(subscription.subscribedDate!)}
+                                      />
+                                    ),
+                                  })}
+                                </div>
+                              </SubscribedDescription>
+                            ) : (
+                              <SubscribedDescription>
+                                {t('Not currently subscribed')}
+                              </SubscribedDescription>
+                            )}
+                          </SubscriptionDetails>
+                          <div>
+                            <Switch
+                              id={`${subscription.email}-${subscription.listId}`}
+                              isActive={subscription.subscribed}
+                              size="lg"
+                              toggle={this.handleToggle.bind(
+                                this,
+                                subscription,
+                                email,
+                                subscription.listId
+                              )}
+                            />
+                          </div>
+                        </PanelItem>
+                      ))}
                   </Fragment>
                 ))}
               </PanelBody>
@@ -191,9 +205,17 @@ const Heading = styled(PanelItem)`
   color: ${p => p.theme.subText};
 `;
 
-const SubscriptionDetails = styled('div')`
-  width: 50%;
+const SubscriptionDetails = styled('label')`
+  font-weight: initial;
   padding-right: ${space(2)};
+  width: 85%;
+
+  @media (min-width: ${p => p.theme.breakpoints.small}) {
+    width: 75%;
+  }
+  @media (min-width: ${p => p.theme.breakpoints.large}) {
+    width: 50%;
+  }
 `;
 
 const SubscriptionName = styled('div')`