#161776762 Edit articles #28

samuelbwambale · 2018-11-15T20:12:32Z

What does this PR do?

Enables users to edit articles which they authored

Description of Task to be completed?

Currently, the users are unable to update articles.

This feature enables users to edit articles. Users can only edit the articles they authored.

How should this be manually tested?

Pull the changes on a new branch and run npm install
run npm start to start the server
Log into the app using a registered email and password
Click on profile and select New Article to add a new article
Select the created article from the list of articles by clicking on Read more link
On loading the article, click on edit button and change any of the fields
Click on save button and see the updated article on the profile

What are the relevant pivotal tracker stories?

#161776762

Screenshots (if appropriate)

Checklist: I completed these to help reviewers

My IDE is configured to follow the ESlint Conventions
I have added tests to cover my changes.
All new and existing tests passed.
My pull request is based on the latest changes of the develop branch.

RonKbS

Nice work @samuelbwambale.
There is a bug that should be easily fixed:
When I click on Read more it shows the article briefly before returning to the dashboard

RonKbS · 2018-11-16T10:14:49Z

src/actions/types.js

 export const CREATE_ARTICLE_SUCCESS = 'CREATE_ARTICLE_SUCCESS';
 export const CREATE_ARTICLE_INITIATED = 'CREATE_ARTICLE_INITIATED';
 export const CREATE_ARTICLE_ERROR = 'CREATE_ARTICLE_ERROR';
 export const LOGOUT_USER = 'LOGOUT_USER';
+export const SEND_RESET_LINK_SUCCESS = 'SEND_RESET_LINK_SUCCESS';


Please remove these reset password changes

@RonKbS Reset-password changes have been removed after rebasing develop branch

@samuelbwambale these particular action_types should also be removed since the updated PR for them has not yet been merged.

@RonKbS The action_types have been removed

MuhanguziDavid · 2018-11-16T10:19:00Z

src/actions/articleActions.js

+        response.data.message,
+        { autoClose: 3500, hideProgressBar: true },
+        {
+          position: toast.POSITION.TOP_CENTER,


@samuelbwambale , please remove this line, the toast position was already set.

@MuhanguziDavid Line removed to take the default toast settings

MuhanguziDavid · 2018-11-16T10:33:02Z

src/components/Articles/ViewArticle.js

+    }
+  }
+
+


Please remove redundant line

@MuhanguziDavid Line removed

MuhanguziDavid · 2018-11-16T10:36:26Z

src/components/Articles/ViewArticle.js

+    const { title, description, body } = this.state;
+    const { loading } = this.props;
+    const { articlePayload, articlePayload: { article } } = this.props;
+    const { isEditing } = this.state;


Lines 92 and 95 can be put on the same line,
i.e. const { title, description, body, isEditing } = this.state;

Lines 93 and 94 can also be put on the same line.

@MuhanguziDavid code refactored as advised

MuhanguziDavid · 2018-11-16T10:38:16Z

src/tests/actions/articleActions.test.js

+    );
+  });
+
+  // it('should not update an article if not the author', async () => {


Please remove the commented block of code

@MuhanguziDavid Commented code has been removed from the PR

samuelbwambale · 2018-11-17T16:47:14Z

@RonKbS the bug causing an article not to be loaded was fixed

coveralls · 2018-11-17T16:47:24Z

Pull Request Test Coverage Report for Build 513

94 of 95 (98.95%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 97.789%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Articles/EditArticle.js	39	40	97.5%

Totals
Change from base Build 500:	0.2%
Covered Lines:	474
Relevant Lines:	479

💛 - Coveralls

RonKbS · 2018-11-19T14:27:48Z

src/actions/articleActions.js

+        errorMessage = 'Re-login and try again';
+      }
+      if (error.response.status === 404) {
+        errorMessage = 'Please enter valid data';


@samuelbwambale I think we may need a more user-friendly message here. Developers might feel at home when they see this message but the same probably can't be said for other types of authors.

I have updated the response to Please enter a valid text. Let me know if this suffices

RonKbS · 2018-11-19T14:32:34Z

src/tests/actions/articleActions.test.js

@@ -185,6 +188,13 @@ describe('likeDislikeAction', () => {
  localStorage.setItem('auth_token', 'token');
  const payload = { reaction: 'Like' };
  const slug = 'testing-1-2-3';
+  const article_data = {
+    article: {
+      body: '<p>The first you’ll want tr, they may have already done this, so  components!</p>',


There are some spelling mistakes here.

@RonKbS This was random text but has been updated to correct spellings

kakaemma · 2018-11-19T20:48:31Z

src/tests/components/NewArticleForm.test.js

+    expect(handleChange).toBeCalled();
+  });
+
+  it('should call handleEditorChange when the there is an input in text editor', () => {


Consider changing when the there on line 47 and 52 to make the test more meaningful.

@kakaemma lines 47 and 52 have been updated with more descriptive comment

kakaemma · 2018-11-19T21:37:30Z

src/components/Articles/NewArticleForm.js

+import ReactQuill from 'react-quill';
+import 'react-quill/dist/quill.snow.css';
+
+


Kindly consider removing this redundant line.

@kakaemma Redundant line has been removed

kakaemma · 2018-11-19T21:45:39Z

src/tests/components/NewArticleForm.test.js

+      loading={loading}
+    />);
+  });
+


This line too should be removed and all the other redundant lines.

@kakaemma Redundant line 31 has been removed. Please note that there are some other blank lines left for easy readability of code.

kakaemma · 2018-11-20T06:59:50Z

src/components/Articles/ViewArticle.js

+    });
+  };
+
+  toggleEdit() {


Consider making this an arrow function which binds automatically to the class. Then remove line 21.

Thanks, @kakaemma toggleEdit() has been redefined as an arrow function and line 21 deleted.

MuhanguziDavid

LGTM

mendozabree

@samuelbwambale I was able to edit an article!
LGTM

RonKbS

LGTM

dondrzzy

Please refactor

dondrzzy · 2018-11-20T11:37:37Z

src/actions/articleActions.js

@@ -62,16 +65,15 @@ export const fetchComments = article => dispatch => {

 export const fetchArticles = () => dispatch => {
  dispatch(getArticlesInitiated(true));
-  return axiosInstance


Why did you remove the return statement?

The return statement has been replaced.

dondrzzy · 2018-11-20T11:38:05Z

src/actions/articleActions.js

    .get('/api/article/')
    .then((response) => {
      dispatch(getAllArticles(response.data.article));
    });
 };

 export const fetchSpecificArticle = slug => dispatch => {
-  dispatch(getArticlesInitiated(true));
-  return axiosInstance


Why did you remove both lines of code?

The getArticlesInitiated(true) was not adding anything to the app. Instead it was disabling the loading of the list of articles. This was removed. Without any other statement in the arrow function, ESLint suggests that the return statement be removed

When you fetch a new article and it takes longer, what does the user see while that happens?

I added a loader to show as the article is being fetched

dondrzzy · 2018-11-20T11:38:49Z

src/actions/articleActions.js

+export const updateArticle = (slug, newData) => dispatch => {
+  toast.dismiss();
+  dispatch(editArticleInititated(true));
+  axiosInstance


Add a return statement. updateArticle should return a promise.

Return statement has been added to return the promise

dondrzzy · 2018-11-20T11:40:05Z

src/actions/articleActions.js

+        errorMessage = 'Please enter valid text';
+      }
+      dispatch(editArticleError(errorMessage));
+      toast.error(errorMessage, { autoClose: false, hideProgressBar: true });


If you are notifying the user of this error, why add it to state?

This dispatch handles change in state for other state variables such as loading besides adding the error to the state

dondrzzy · 2018-11-20T11:42:41Z

src/components/Articles/ViewArticle.js

+    const { article: { title, body, description } } = articlePayload;
+    this.setState({ title });
+    this.setState({ body });
+    this.setState({ description });


Use one setState statement.

This piece of code has been refactored as advised

dondrzzy · 2018-11-20T11:45:06Z

src/tests/components/NewArticle.test.js

+      addArticle={addArticle}
+      resetForm={resetForm}
+      handleEditorChange={handleEditorChange}
+      handleChange={handleChange}


Add all these props to the props const.

The constants have been added into a single object const props {}

dondrzzy · 2018-11-20T11:45:52Z

src/tests/components/NewArticleForm.test.js

+  const title = '';
+  const description = '';
+  const body = '';
+  const loading = false;


Use a props object for all these props and pass its spread.

The constants have been added into a single object const props {}

dondrzzy · 2018-11-20T11:47:06Z

src/tests/components/NewArticleForm.test.js

+
+  it('should call handleSubmit when the form is submitted', () => {
+    wrapper.find('#add-article-form').simulate('submit');
+    expect(handleSubmit).toBeCalled();


Add a test showing what describe handleSubmit does. Same goes for the rest

This was handled in the file NewArticle.test.js which has the tests for the main component

dondrzzy · 2018-11-20T11:47:19Z

src/tests/components/NewArticleForm.test.js

+
+  it('should call resetForm when the clear button is clicked', () => {
+    wrapper.find('#clear-button').simulate('click', { preventDefault() {} });
+    expect(resetForm).toBeCalled();


Add a test showing what describe resetForm does.

This was handled in the file NewArticle.test.js which has the tests for the main component

dondrzzy

Please refactor

dondrzzy · 2018-11-21T14:41:11Z

src/components/Articles/EditArticle.js

+
+  componentWillReceiveProps(nextProps) {
+    const { articlePayload } = nextProps;
+    if (nextProps.articlePayload !== this.props.articlePayload) {


Did you intentionally ignore this linting error?

This was skipped accidentally. It has been restructured.

dondrzzy · 2018-11-21T14:43:40Z

src/components/Articles/EditArticle.js

+    const { loading, articlePayload, articlePayload: { article } } = this.props;
+    if (isEditing) {
+      return (
+        <CreateArticleForm


Rename this to ArticleForm. Create does not sound right in Edit.

CreateArticleForm has been renamed to ArticleForm

dondrzzy · 2018-11-21T14:48:03Z

src/actions/articleActions.js

+        errorMessage = 'Re-login and try again';
+      }
+      if (error.response.status === 404) {
+        errorMessage = 'Please enter valid text';


Does valid text help the user determine whether its the title, body or description with the issue? Is there an error message that returns with this error response?

The front-end already caters for validation of title and description fields. I have updated the message to point to the article body specifically.

dondrzzy · 2018-11-21T14:52:15Z

src/tests/reducers/articleReducer.test.js

+      payload: true,
+    };
+    const currentState = articlesReducer(initialState, action);
+    expect(currentState).toEqual({


Everyone seems to be doing this which is making this file ambiguous. How about this:

expect(...).toEqual({
...initialState,
loading: action.payload,
});
@samuelbwambale . If it works, advise the next victims.

I have refactored this and shared with the team

- Enable users to edit articles they authored [Delivers #161776762]

RonKbS reviewed Nov 15, 2018

View reviewed changes

RonKbS reviewed Nov 16, 2018

View reviewed changes

MuhanguziDavid reviewed Nov 16, 2018

View reviewed changes

samuelbwambale force-pushed the ft-edit-article-161776762 branch from 7912c35 to 81f206e Compare November 16, 2018 10:19

MuhanguziDavid reviewed Nov 16, 2018

View reviewed changes

samuelbwambale force-pushed the ft-edit-article-161776762 branch from 81f206e to 7047ed0 Compare November 17, 2018 16:44

samuelbwambale force-pushed the ft-edit-article-161776762 branch 3 times, most recently from f43385c to a419314 Compare November 19, 2018 10:00

samuelbwambale requested review from MuhanguziDavid, mendozabree and RonKbS November 19, 2018 11:53

samuelbwambale force-pushed the ft-edit-article-161776762 branch 2 times, most recently from b1847bc to 2f0f051 Compare November 19, 2018 14:26

RonKbS reviewed Nov 19, 2018

View reviewed changes

samuelbwambale force-pushed the ft-edit-article-161776762 branch 3 times, most recently from 503ef0c to f930662 Compare November 19, 2018 17:12

kakaemma reviewed Nov 19, 2018

View reviewed changes

kakaemma reviewed Nov 20, 2018

View reviewed changes

samuelbwambale force-pushed the ft-edit-article-161776762 branch from f930662 to eed17cb Compare November 20, 2018 07:41

MuhanguziDavid approved these changes Nov 20, 2018

View reviewed changes

mendozabree approved these changes Nov 20, 2018

View reviewed changes

RonKbS approved these changes Nov 20, 2018

View reviewed changes

samuelbwambale force-pushed the ft-edit-article-161776762 branch from eed17cb to 265c750 Compare November 20, 2018 09:52

dondrzzy reviewed Nov 20, 2018

View reviewed changes

samuelbwambale force-pushed the ft-edit-article-161776762 branch 3 times, most recently from 1bce059 to 7316f39 Compare November 21, 2018 10:12

dondrzzy reviewed Nov 21, 2018

View reviewed changes

samuelbwambale force-pushed the ft-edit-article-161776762 branch from 7316f39 to a7d006b Compare November 22, 2018 09:20

feature(edit articles): users can edit articles

8a21670

- Enable users to edit articles they authored [Delivers #161776762]

samuelbwambale force-pushed the ft-edit-article-161776762 branch from a7d006b to 8a21670 Compare November 22, 2018 09:21

dondrzzy merged commit 1c3190b into develop Nov 22, 2018

		import ReactQuill from 'react-quill';
		import 'react-quill/dist/quill.snow.css';

#161776762 Edit articles #28

#161776762 Edit articles #28

Conversation

samuelbwambale commented Nov 15, 2018 • edited Loading

What does this PR do?

Description of Task to be completed?

How should this be manually tested?

What are the relevant pivotal tracker stories?

Screenshots (if appropriate)

Checklist: I completed these to help reviewers

RonKbS 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

samuelbwambale commented Nov 17, 2018

coveralls commented Nov 17, 2018 • edited Loading

Pull Request Test Coverage Report for Build 513

💛 - Coveralls

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

kakaemma Nov 19, 2018 • edited Loading

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

MuhanguziDavid left a comment

Choose a reason for hiding this comment

mendozabree left a comment

Choose a reason for hiding this comment

RonKbS left a comment

Choose a reason for hiding this comment

dondrzzy 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

samuelbwambale Nov 20, 2018 • edited Loading

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

samuelbwambale Nov 20, 2018 • edited Loading

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

samuelbwambale Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelbwambale Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

dondrzzy 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

samuelbwambale commented Nov 15, 2018 •

edited

Loading

coveralls commented Nov 17, 2018 •

edited

Loading

kakaemma Nov 19, 2018 •

edited

Loading

samuelbwambale Nov 20, 2018 •

edited

Loading

samuelbwambale Nov 20, 2018 •

edited

Loading

samuelbwambale Nov 20, 2018 •

edited

Loading

samuelbwambale Nov 20, 2018 •

edited

Loading