OCLOMRS-44: A user should be able to view all dictionaries #34

EleisonC · 2018-06-18T20:56:49Z

JIRA TICKET NAME:

Summary:

A user should be able to retrieve all dictionaries both the one created by them and are private to them and all other public dictionaries

coveralls · 2018-06-18T21:00:32Z

Pull Request Test Coverage Report for Build 175

26 of 34 (76.47%) changed or added relevant lines in 8 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.2%) to 68.016%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/dashboard/components/dictionary/ListDictionaries.jsx	5	8	62.5%
src/redux/actions/dictionaries/dictionaryActionCreators.js	4	9	44.44%

Files with Coverage Reduction	New Missed Lines	%
src/components/dashboard/components/ListWrapper.jsx	1	83.33%
src/components/dashboard/components/dictionary/AddDictionary.jsx	1	20.0%

Totals
Change from base Build 161:	0.2%
Covered Lines:	258
Relevant Lines:	351

💛 - Coveralls

mwaz · 2018-06-19T07:44:55Z

src/components/dashboard/components/dictionary/ListDictionaries.jsx

+        dictionaryName: PropTypes.string,
+    })).isRequired,
+};
+export default ListDictionaries;


new line required here

mwaz · 2018-06-19T07:45:31Z

src/redux/actions/dictionaries/dictionaryActionCreators.js

+          dispatch(isFetching(false));
+      }
+  }
+}


new line required

mwaz · 2018-06-19T07:46:05Z

src/tests/__mocks__/dictionaries.jsx

+    "references": [],
+    "active_concepts": 0,
+    "active_mappings": 0
+}


new line required here

will work on that

mwaz · 2018-06-19T07:47:17Z

src/components/dashboard/components/SideNavigation.jsx

 import AddDictionary from './dictionary/AddDictionary';
-
+import{ Link }from 'react-router-dom'


eslint spaces not present on the import, also missing a semicolon

I will deal with that thank you for the feedback

mwaz · 2018-06-19T07:52:12Z

src/components/dashboard/styles/index.css

+  font-size: 11px;
+  border: none;
+}


There is a dictionary css file, i think it might make more sense if all the css for dictionaries is in that file rather than in the index file of the dashboard.

The reason I placed that there, is I am reusing the CSS that Emma had written hence avoid repetition of code. In the dictionary CSS file

mwaz · 2018-06-19T07:58:22Z

src/redux/actions/dictionaries/dictionaryActionCreators.js

@@ -27,3 +28,25 @@ export const fetchingOrganizations = () => dispatch =>
    .fetchOrganizations()
    .then(payload =>
      dispatch(fetchOrganizations(payload)));
+
+export const fetchDictionaries = (


maybe we should get some member contribution on the same but i think this would look much neater in the api.js file.
Something like
export const fetchDictionaries = (query, limit, page) => dispatch => api.dictionaries .fetchDictionaries(query, limit, page)
then you can copy everything else from here to fetchDictionaries function in api.js. (This is just but a suggestion just to keep the consistency)

mwaz · 2018-06-19T08:01:54Z

apart from the minor changes, PR looks solidly good

hadijahkyampeire · 2018-06-19T08:50:27Z

src/tests/Dashboard/container/SourceSearch.test.jsx

@@ -17,7 +18,7 @@ describe('Dashboard Component', () => {
      clearSources: jest.fn(),
      hasMore: false,
    };
-    const wrapper = mount(<SourceSearch {...props} />);
+    const wrapper = shallow(<MemoryRouter><SourceSearch {...props} /></MemoryRouter>);


Why did you have to change this file, I thought you were to work on dictionaries only

I made changes to the SideNavigation.jsx file to use the Link element hence I had to wrap the component being tested in the MemoryRouter

Yah and that should be another ticket I guess because I had also changed anchors to Links and someone commented on that, or maybe change only yours to avoid many irrelevant changes as per this ticket

This would leave all the other test files failing as a result of rendering the Link out of the MemoryRouter

hadijahkyampeire · 2018-06-19T08:51:11Z

src/tests/Dashboard/container/Concepts.test.jsx

@@ -15,7 +16,7 @@ describe('Dashboard Concept Component', () => {
      concepts: [],
      isFetching: false,
    };
-    const wrapper = mount(<ConceptSearch {...props} />);
+    const wrapper = mount(<MemoryRouter><ConceptSearch {...props} /></MemoryRouter>);


why did you have to change this file

I made changes to the SideNavigation.jsx file to use the Link element hence I had to wrap the component being tested in the MemoryRouter

Same here, you can only change for your link so as to stay focused to your ticket.

it's part of the sideNavigation component which is rendered in all containers that are tested

hadijahkyampeire · 2018-06-19T08:51:29Z

src/redux/reducers/index.js

@@ -3,13 +3,16 @@ import AuthReducers from './authReducers';
 import sources from './sourcesReducer';
 import organizations from './dictionaryReducer';
 import concepts from './ConceptReducers';
+import dictionaries from './dictionaryReducers';
+


Aren't we leaving many lines than required here?

I will deal with that thank you

hadijahkyampeire · 2018-06-19T08:51:44Z

src/redux/actions/dictionaries/dictionaryActionCreators.js

+          dispatch(isFetching(false));
+      }
+  }
+}


same here new line

hadijahkyampeire · 2018-06-19T08:52:07Z

src/components/dashboard/components/dictionary/ListDictionaries.jsx

+        dictionaryName: PropTypes.string,
+    })).isRequired,
+};
+export default ListDictionaries;


new line needed here

It seems this wasn't resolved

It was resolved

hadijahkyampeire · 2018-06-19T08:53:31Z

src/components/dashboard/components/SideNavigation.jsx

          </li>
+        <li className="nav-item">


why did you put dictionaries in their own 'li' why not follow the convention

because it is a list item

OCLOMRS-44: A user should be able to view all dictionaries OCLOMRS-44: A user should be able to view all dictionaries OCLOMRS-44: A user should be able to view all dictionaries OCLOMRS-44: A user should be able to view all dictionaries OCLOMRS-44: A user should be able to view all dictionaries

emasys · 2018-06-19T19:11:25Z

src/components/dashboard/components/dictionary/AddDictionary.jsx


 export class AddDictionary extends React.Component {
  submit = data => (
    data.owner === 'Individual' ? this.props.createDictionaryUser(data).then(this.props.handleHide)
-      : this.props.createDictionary(data).then(this.props.handleHide))
+      : this.props.createDictionary(data).then(this.props.handleHide)).then(this.props.fetchDictionaries())


These promises should not be resolved in the component, you should create an action creator (thunk) for this.

What do you think about making a new ticket to fix that because that particular file was created by someone else and what I added was their suggestion to update state after creating a new dictionary

shakirandagire

LGTM

mwaz · 2018-06-20T08:12:00Z

LGTM

hadijahkyampeire

LGTM

EleisonC force-pushed the OCLOMRS-44 branch from 1af8ef3 to 9e048e7 Compare June 19, 2018 06:46

mwaz reviewed Jun 19, 2018

View reviewed changes

src/redux/actions/dictionaries/dictionaryActionCreators.js Outdated

dispatch(isFetching(false));

}

}

}

Copy link

Contributor

mwaz Jun 19, 2018

Choose a reason for hiding this comment

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

new line required

mwaz reviewed Jun 19, 2018

View reviewed changes

hadijahkyampeire reviewed Jun 19, 2018

View reviewed changes

EleisonC force-pushed the OCLOMRS-44 branch 3 times, most recently from 99268f6 to 1d163a8 Compare June 19, 2018 16:39

EleisonC force-pushed the OCLOMRS-44 branch from 1d163a8 to b73088b Compare June 19, 2018 17:18

emasys suggested changes Jun 19, 2018

View reviewed changes

emasys approved these changes Jun 20, 2018

View reviewed changes

shakirandagire approved these changes Jun 20, 2018

View reviewed changes

hadijahkyampeire approved these changes Jun 20, 2018

View reviewed changes

dkayiwa merged commit 583ef9f into openmrs:master Jun 20, 2018

		import AddDictionary from './dictionary/AddDictionary';

		import{ Link }from 'react-router-dom'

OCLOMRS-44: A user should be able to view all dictionaries #34

OCLOMRS-44: A user should be able to view all dictionaries #34

Conversation

EleisonC commented Jun 18, 2018

JIRA TICKET NAME:

Summary:

coveralls commented Jun 18, 2018 • edited Loading

Pull Request Test Coverage Report for Build 175

💛 - 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

mwaz Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz Jun 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

mwaz Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz commented Jun 19, 2018

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

hadijahkyampeire Jun 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

shakirandagire left a comment

Choose a reason for hiding this comment

mwaz commented Jun 20, 2018

hadijahkyampeire left a comment

Choose a reason for hiding this comment

coveralls commented Jun 18, 2018 •

edited

Loading

mwaz Jun 19, 2018 •

edited

Loading

mwaz Jun 19, 2018 •

edited

Loading

mwaz Jun 19, 2018 •

edited

Loading

hadijahkyampeire Jun 19, 2018 •

edited

Loading