Integrate StatusAlert into Delete Workflow #40

fysheets · 2017-10-18T20:41:45Z

Integrate the proper Paragon StatusAlert to appear on success or failure of an asset being deleted.

fysheets · 2017-10-19T13:25:52Z

src/data/constants/assets.js

@@ -0,0 +1,3 @@
+/* ASSETS RESPONSE MESSAGES */
+export const ERROR_FILE_DELETE = 'File could not be deleted.';
+export const SUCCESS_FILE_DELETE = 'File was successfully deleted.';


Do we want the messages for responses to be more descriptive? Do we also want to store other static messages (i.e modal body text / confirmation button text / status alert text in this constants file as well, or do we prefer to have that inline? Is any of it reusable on other pages that we may do in the future?

Also this file was created because of the use of checking the status message to determine if the request being returned by the action creator was a success or failure, moving the constant string to a shared location reduces the number of places that will need to get changed in the future if we decide to change it. This checking of the message currently determines where focus should move from the StatusAlert.

We're going to need to consider i18n with these strings as well - the "inline vs centralized" string storage decision will need to be made and standardized across the repo soon.

fysheets · 2017-10-19T13:28:32Z

src/data/reducers/assets.js

@@ -23,6 +23,13 @@ const list = (state = [], action) => {

 const status = (state = {}, action) => {
  switch (action.type) {
+    case assetActions.CLEAR_ASSETS_STATUS:
+      return {};


I added this because the status message is what is being used to determine the styling of the StatusAlert -- there is a short amount of time where the alert will show up before the delete request has completed and returned a response, this could cause the styling to show incorrectly briefly (green first to red failure or vice versa). By clearing the status after the close of the alert, the next time the alert is triggered it will show the warning/"Deleting asset" text before showing the correct status (yellow to red/green).

fysheets · 2017-10-19T13:30:53Z

src/data/actions/assets.js

@@ -42,9 +46,12 @@ export const deleteAsset = (assetsParameters, assetId) =>
    clientApi.requestDeleteAsset(assetsParameters.courseId, assetId)
      .then((response) => {
        if (response.ok) {
-          dispatch(deleteAssetSuccess(assetId));
+          dispatch(deleteAssetSuccess(assetId, response, AssetsConstants.SUCCESS_FILE_DELETE));


It makes sense to me that we would send the response details on each response completion (success or failure). Even if we don't do anything with the response body, I think returning back to redux the updated status of success or failure on call is good practice. Also if we ever do want to track our calls for success/failure in the future we are already set up for collecting that information.

Should pose the question -- do we want to do this in this manner where the SUCCESS_FILE_DELETE action type is used in two different reducers, or do we want two dispatch separate dispatch calls and create a new assetXHRSuccess / ASSET_XHR_SUCCESS type? I feel like the way I did it isn't recommended even if it does technically work. It might also be more clear if we do two separate dispatch calls? @arizzitano specific react/redux framework question

Ok so there's a lot going on here.

I'm fine passing the whole response payload back to the reducer. Fat vs skinny action creators/reducers is a matter of debate. Either way, we should make sure the reducer only stores the fields we care about, not the entire payload -- this will eat up browser memory.

I'm team use-the-same-action-type-in-multiple-reducers. IMO this is one of the most powerful features of Redux, and Abramov himself pushes it.

Seems like we're getting to the point of feeling XHR pain and maybe needing to start thinking about a layer of abstraction. I don't think you need to do it in this PR (we're not there QUITE yet) but we need to start thinking about it. i started typing up a proposed implementation but realized it would be better as a PR so adding that to my backlog.

fysheets · 2017-10-19T13:33:35Z

src/AssetsPage/AssetsTable/index.jsx

+    return focusRowButton;
+  }
+
+  addSupplementalTableElements() {


Renamed this fuction as it now has more use cases than just adding the delete button -- will add lock button and now adds an ID for the row # that the data is on. That row # is used for determining the next focusable element once the status alert is closed.

dsjen

Nice tests! I left a few questions for you. Thanks!

dsjen · 2017-10-23T15:39:49Z

src/AssetsPage/AssetsTable/index.jsx

+      focusRowButton = this.trashcanRefs[focusAsset.id];
+    }
+
+    return focusRowButton;


Nit: I wonder if focusRowButton can be removed entirely and for the return to just be:

return this.trashcanRefs[focusAsset.id];

Just to be clear, I have no qualms with how this is written as it's quite understandable, and the suggestion is just to make it a bit tighter.

Definitely -- good catch on some clean up.

dsjen · 2017-10-23T15:42:55Z

src/AssetsPage/AssetsTable/index.jsx

 import classNames from 'classnames';
 import { connect } from 'react-redux';

 import FontAwesomeStyles from 'font-awesome/css/font-awesome.min.css';
-import { deleteAsset } from '../../data/actions/assets';
-
+import { deleteAsset, clearAssetsStatus } from '../../data/actions/assets';


Question: do we care for these to be in alphabetical order? It's definitely a preference in the platform. Personally, I don't have strong opinions.

I was not aware of the preference, but I'm all for keeping some sort of convention across repos, I'll swap that to be alphabetical.

Ok! Either way works for me, but I just wanted to check on that just for my own sake mostly. :)

dsjen · 2017-10-23T18:45:49Z

src/AssetsPage/AssetsTable/AssetsTable.test.jsx

@@ -41,3 +48,97 @@ describe('<AssetsTable />', () => {
    expect(wrapper.find('tr')).toHaveLength(defaultProps.assetsList.length + 1);
  });
 });
+
+describe('it displays alert properly', () => {
+  it('renders info alert with blank status', () => {


I'm not sure that this is testing the blank status. Should it be checking that the text is empty? Maybe the test is misnamed?

Good catch! It does not check the status message, it is misnamed.

dsjen · 2017-10-23T18:51:52Z

src/AssetsPage/AssetsTable/AssetsTable.test.jsx


+const setAssetToDelete = (asset, wrapper) => {


Is this pulled out to a function for clarity? Just wondering!

I figured we may want to test this in other tests moving forward so I moved it out to reduce the duplication that I was already doing - it could easily go back into each test, I wasn't sure which was preferable for testing. I could see it making sense to have it in each since they're supposed to be individual tests, but this seemed cleaner for the duplication.

dsjen · 2017-10-23T18:53:24Z

src/AssetsPage/AssetsTable/AssetsTable.test.jsx

 };

+let wrapper;


I always get sort of nervous about globals. It seems like each use of wrapper could be declared with let to scope it within the test.

In all fairness this was predefined when I got into this file - I just had to move it's location below the setAssetToDelete from above. I don't mind changing it from the global to individual declarations, I just wasn't the one to define it this way initially so I'm not sure if it was done for a reason?

Ah, I see! I guess I'd personally not do it globally and would probably make that change.

I'm not super opinionated about this personally but Dennis makes a good point.

dsjen · 2017-10-23T18:58:06Z

src/AssetsPage/AssetsTable/index.jsx

@@ -48,19 +53,40 @@ export class AssetsTable extends React.Component {
    });
  }

-  addDeleteButton() {
-    const newAssetsList = this.props.assetsList.map((asset) => {
+  getNextFocusElementOnDelete(assetToDelete) {


Maybe getNextFocusElementOnDeleteClose? That's so long, but I think clearer.

Done! And agreed, it is clearer.

Actually, after reading more of your comments -- it's determining where to focus after closing the StatusAlert that appears on delete. I'm not sure if the OnDeleteClose is clear about that, but if we're going to be using the status alert for multiple alert types (delete/copy URL/...) I'm not sure if OnAlertClose will be clear either because that will have different rules, though I guess we could maybe extend that function to encompass all the rules related to the alert and focus?

Ah, I see! Maybe the current name makes the most sense actually. I'm reading it again and finding it to be clear!

dsjen · 2017-10-23T19:02:30Z

src/AssetsPage/AssetsTable/index.jsx

+    let focusAsset = assetToDelete;
+    let focusRowButton = this.trashcanRefs[assetToDelete.id];
+
+    if (this.props.assetsStatus.text === AssetsConstants.SUCCESS_FILE_DELETE) {


Is this function (in theory) ever called if this isn't the case? Also, the comparison of text is a bit worrisome especially in the case that the text is translated. Is there some sort of unique state/status/prop/code/you-get-the-gist that we could use instead?

Yes, it will still be called because regardless of the response status we need to know where to send focus when the status alert is closed out of. The text comparison is using the response body text, not text that will ever been shown to the user / internationalized. (The ones that now live in the constants/assets.js file). But I understand the concern, I'll test out if I can use response code or something else here.

Oh, I see! I think that assetsStatus has a text field is what confused me. That's your status code, right? I don't want to bloat your PR, but I think that if text were renamed to statusCode or type (or something else), it would be clearer. To me, text indicates that it's displayed to the user.

dsjen · 2017-10-23T19:04:04Z

src/AssetsPage/AssetsTable/index.jsx

@@ -104,7 +138,7 @@ export class AssetsTable extends React.Component {
    );
  }

-  renderBody() {
+  renderModalBody() {


I do think this is clearer. 🍨

dsjen · 2017-10-23T19:11:04Z

src/AssetsPage/AssetsTable/index.jsx

+    let alertType = 'info';
+
+    if (assetsStatus.text === AssetsConstants.ERROR_FILE_DELETE) {
+      alertDialog = `Unable to delete ${deleteAssetName}.`;


Not something to worry about now, but we'll need for this to be internationalized too (e.g. string filled in via template) or some way to do this. CC: @efischer19

👍 Agreed. We'll also need it for the Modal body text & buttons.

codecov-io · 2017-10-23T21:47:45Z

Codecov Report

Merging #40 into master will increase coverage by 4.41%.
The diff coverage is 74.46%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   64.81%   69.23%   +4.41%     
==========================================
  Files           4        5       +1     
  Lines          54       91      +37     
  Branches        3        6       +3     
==========================================
+ Hits           35       63      +28     
- Misses         18       27       +9     
  Partials        1        1

Impacted Files	Coverage Δ
src/data/constants/actionTypes.js	`100% <100%> (ø)`
src/data/actions/assets.js	`38.88% <42.85%> (-4.87%)`	⬇️
src/AssetsPage/AssetsTable/index.jsx	`75.75% <79.48%> (+3.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74f9836...7a2a98f. Read the comment docs.

dsjen · 2017-10-24T18:19:07Z

src/data/reducers/assets.js

+    case assetActions.DELETE_ASSET_SUCCESS:
+      return {
+        response: action.response,
+        text: action.text,


Recap of our in-person conversation:

we'll use the existing type instead of text to compare against

the type constants will be moved into a separate file

Thanks for the discussion!

dsjen

LGTM! Thank you for talking through the PR with me; I'm learning! 🎖

dsjen · 2017-10-24T20:55:13Z

src/data/constants/actionTypes.js

@@ -0,0 +1,9 @@
+/* ASSETS ACTION TYPES */


arizzitano

Looks good, sorry this took so long -- 🚢, and could you give it a minor version bump also?

arizzitano · 2017-10-24T16:34:49Z

src/AssetsPage/AssetsTable/AssetsTable.test.jsx

 };

+let wrapper;


I'm not super opinionated about this personally but Dennis makes a good point.

arizzitano · 2017-10-24T18:24:55Z

src/data/actions/assets.js

@@ -42,9 +46,12 @@ export const deleteAsset = (assetsParameters, assetId) =>
    clientApi.requestDeleteAsset(assetsParameters.courseId, assetId)
      .then((response) => {
        if (response.ok) {
-          dispatch(deleteAssetSuccess(assetId));
+          dispatch(deleteAssetSuccess(assetId, response, AssetsConstants.SUCCESS_FILE_DELETE));


Ok so there's a lot going on here.

I'm fine passing the whole response payload back to the reducer. Fat vs skinny action creators/reducers is a matter of debate. Either way, we should make sure the reducer only stores the fields we care about, not the entire payload -- this will eat up browser memory.

I'm team use-the-same-action-type-in-multiple-reducers. IMO this is one of the most powerful features of Redux, and Abramov himself pushes it.

Seems like we're getting to the point of feeling XHR pain and maybe needing to start thinking about a layer of abstraction. I don't think you need to do it in this PR (we're not there QUITE yet) but we need to start thinking about it. i started typing up a proposed implementation but realized it would be better as a PR so adding that to my backlog.

arizzitano · 2017-10-24T19:37:43Z

src/AssetsPage/AssetsTable/index.jsx

+    return this.trashcanRefs[focusAsset.id];
+  }
+
+  addSupplementalTableElements() {


Better name. This is going to be a big source of conflicts with @MichaelRoytman's PR though so once it merges the three of us should circle back at some point and talk about his changes.

efischer19

New tests LGTM

dsjen · 2017-10-25T17:15:58Z

🙌 🍰 💯

fysheets commented Oct 19, 2017

View reviewed changes

fysheets changed the title ~~WIP: Integrate StatusAlert into Delete Workflow~~ Integrate StatusAlert into Delete Workflow Oct 19, 2017

fysheets force-pushed the fsheets/delete-status-alert branch 2 times, most recently from 5bc6ad5 to fbdc725 Compare October 23, 2017 17:16

dsjen reviewed Oct 23, 2017

View reviewed changes

dsjen reviewed Oct 24, 2017

View reviewed changes

fysheets force-pushed the fsheets/delete-status-alert branch from 7aaf728 to e5a813c Compare October 24, 2017 20:16

dsjen approved these changes Oct 24, 2017

View reviewed changes

src/data/constants/actionTypes.js

@@ -0,0 +1,9 @@

/* ASSETS ACTION TYPES */

Copy link

Contributor

dsjen Oct 24, 2017

Choose a reason for hiding this comment

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

😍

arizzitano approved these changes Oct 25, 2017

View reviewed changes

efischer19 approved these changes Oct 25, 2017

View reviewed changes

Add StatusAlert on Delete of Asset

7a2a98f

fysheets force-pushed the fsheets/delete-status-alert branch from 43e02ff to 7a2a98f Compare October 25, 2017 16:25

fysheets merged commit 0a4ef1d into master Oct 25, 2017

fysheets deleted the fsheets/delete-status-alert branch November 7, 2017 18:40

Integrate StatusAlert into Delete Workflow #40

Integrate StatusAlert into Delete Workflow #40

Conversation

fysheets commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsjen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 23, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dsjen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arizzitano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efischer19 left a comment

Choose a reason for hiding this comment

dsjen commented Oct 25, 2017

codecov-io commented Oct 23, 2017 •

edited

Loading