OCLOMRS-967: Pick concepts from a dictionary (in addition to sources) #712

jwnasambu · 2021-06-30T22:04:39Z

JIRA TICKET NAME:

Pick concepts from a dictionary (in addition to sources)

Summary:

I worked on a feature that allows a user to create a dictionary that contains concepts from another dictionary.

coveralls · 2021-06-30T22:15:54Z

Coverage increased (+0.2%) to 46.72% when pulling 65f8860 on jwnasambu:OCLOMRS-967 into 02c0d02 on openmrs:master.

ibacher

I've added quite a few comments, including one that should fix the build error.

src/apps/concepts/components/ViewConceptsHeader.tsx

ibacher · 2021-07-01T17:37:50Z

src/apps/concepts/components/ViewConceptsHeader.tsx

-    underline: {
-      "&&&:before": {
-        borderBottom: "none"
-      },
-      "&&:after": {
-        borderBottom: "none"
-      }
-    },


I assume this should be restored as well?

ibacher · 2021-07-01T17:38:31Z

src/apps/concepts/components/ViewConceptsHeader.tsx

+    dictionaryIcon:{
+      marginRight: "0.2rem",
+      fill: "#8080809c"


We don't really need a separate dictionaryIcon class... This can be shared with sourceIcon (at least until we develop a need to style the icons differently)

ibacher · 2021-07-01T17:38:44Z

src/apps/concepts/components/ViewConceptsHeader.tsx

@@ -36,24 +38,21 @@ const useStyles = makeStyles((theme: Theme) =>
    },
    textField: {
      padding: "0.2rem 1rem",
-      cursor: "none"
+      cursor: "none",
+      width: "100%"


Why this change?

ibacher · 2021-07-01T17:40:51Z

src/apps/dictionaries/redux/actions.ts

+      sourceUrl,
+      false


This is in the wrong order. sourceUrl should be the last argument.

Also, we should do away with this copy of recursivelyFetchToConcepts() since we're using that below, right?

src/apps/dictionaries/redux/actions.ts

ibacher · 2021-07-01T17:42:44Z

src/apps/dictionaries/redux/actions.ts

    );

+    const groupedConcepts = groupBy(concepts.sourceUrl),


Suggested change

const groupedConcepts = groupBy(concepts.sourceUrl),

const groupedConcepts = groupBy(concepts.sourceUrl);

ibacher · 2021-07-01T17:44:50Z

src/apps/dictionaries/redux/actions.ts

    );

+    const groupedConcepts = groupBy(concepts.sourceUrl),
+     const referencesToAdd = Object.entries(groupedConcepts).map(([source, concepts]) => recursivelyFetchToConcepts(
+       concepts.sourceUrl,


The sourceUrl should be the source argument. Remember, however, that the sourceUrl now needs to be the last argument to this, if we're re-arranging the argument order for recursivelyFetchToConcepts()

ibacher · 2021-07-01T17:45:26Z

src/apps/sources/__test__/redux/actions.test.ts

@@ -78,7 +78,7 @@ describe("action", () => {
    expect(indexedAction).toHaveBeenCalledWith("sources/retrieveSources", 1);
  });

-  it("for public sources should use action index as 2 along with the corresponding actionType and api", () => {
+  it("for All Public Concepts should use action index as 2 along with the corresponding actionType and api", () => {


Don't replace this text... We only need things to read "All Public Concepts" in the UI or where we're testing something about the UI.

ibacher · 2021-07-01T17:45:41Z

src/apps/sources/__test__/utils.test.ts

@@ -1,9 +1,9 @@
 import { getSourceTypeFromPreviousPath } from "../utils";

 describe("getSourceTypeFromPreviousPath", () => {
-  it("should return public sources for /sources/", () => {
+  it("should returnAll Public Concepts for /sources/", () => {


Ditto on not needing "All Public Concepts" here

@ibacher thanks for the review. Let me fix the proposed changes.

hadijahkyampeire · 2021-07-06T11:15:29Z

@jwnasambu why do we have two PRS for this work, can you please close one and maintain one?

jwnasambu · 2021-07-06T12:59:36Z

@hadijahkyampeire sorry for the confusion caused! After shifting to a new pr I forgot to close the later. Hopefully its fixed now.

hadijahkyampeire · 2021-07-07T12:52:02Z

@jwnasambu we can still see two PRs for this work, please close one that you are not working on and maintain one which we can review.

This is meant to cover the case where unit tests fail

ibacher

@jwnasambu I've added some comments on a few points here. I'm not entirely sure this is all the changes necessary, but please try them out an let me know.

ibacher · 2021-07-17T19:19:43Z

src/apps/concepts/components/EnhancedTableToolbar.tsx

-          {!showAddConcepts ? null : (
+          {showAddConcepts ? null : (


I see what you're doing here, but this should actually still be:

{!showAddConcepts ? null : (

Because it's still possible to be viewing concepts without a dictionary to add them to

ibacher · 2021-07-17T19:26:39Z

src/apps/dictionaries/redux/actions.ts

+    // const referencesToAdd = await recursivelyFetchToConcepts(
+     // concepts.map(concept => concept.id),
+     // updateProgress,
+    //  false,
+     // sourceUrl
+   // );
+
+   const referencesToAdd = await recursivelyFetchToConcepts(
+  '/sources/CIEL/',
+   concepts.map(concept => concept.id),
+  updateProgress
+  );
+
+    const groupedConcepts = groupBy(concepts, "source");
+    console.log(groupedConcepts)
+    // const referencesToAdd = await Object.entries(groupedConcepts).map(([source, concepts]) => recursivelyFetchToConcepts(
+    //  source,
+    //  concepts.map((concept: { id: any; }) => concept.id),
+    // updateProgress,1,
+    //  ));


Alright so this is very confusing, but here's what I think the implementation should look like:

const groupedConcepts = groupBy(concepts, "source_url"); const referencesToAdd = flatten( await Promise.all( Object.entries(groupedConcepts).map(([source, concepts]) => recursivelyFetchToConcepts( source, concepts.map((concept) => concept.id), updateProgress ) ) ) );

The trick here is that by calling map() we're building an array of... whatever the return value is. In this case, it's the return value of the call to recursivelyFetchToConcepts() which actually returns a Promise<string[]> (a Promise to return an array of strings at a later point).

In the currently working version, the await before the recursivelyFetchToConcepts() waits until the call to recursivelyFetchToConcepts() returns. Since we're returning an array of promises, (one for each call to recursivelyFetchToConcepts(), i.e., one for each source), we need to wait until all those promises are finished. Promise.all() allows us to do this, turning an array of Promises into a promise that resolves with an array of results. We can safely await that.

Finally, I used lodash's flatten function to transform the return time from string[][] (an array of arrays of strings) to a single array, that should look much like the one that existed before all these changes.

ibacher · 2021-07-17T19:28:34Z

src/apps/dictionaries/redux/actions.ts

  dictionaryUrl: string,
  rawConcepts: (APIConcept | string)[],
-  bulk: boolean = false
+  bulk: boolean = false,
+  sourceUrl?: string
 ) => {
  return async (dispatch: Function, getState: Function) => {
    const concepts = rawConcepts.map(concept =>


To address my previous concern, this should look like this:

if (!!!sourceUrl && !!rawConcepts.find((c) => typeof c === "string")) { throw { message: "Cannot load string-only concepts without a source url", }; } const concepts = rawConcepts.map((concept) => typeof concept === "string" ? { id: concept, url: `${sourceUrl}concepts/${concept}/`, source_url: sourceUrl, } : concept );

Note that this throws an error if rawConcepts contains any strings an no sourceUrl was provided. It also adds the source_url property to our concepts so we can use this in the groupBy below.

src/apps/dictionaries/redux/actions.ts

ibacher · 2021-07-19T15:21:21Z

src/apps/dictionaries/redux/actions.ts

+function getState(): import("../../../redux").AppState {
+  throw new Error("Function not implemented.");
+}
+
+function dispatch(arg0: { type: string; actionIndex: number; meta: any[]; }) {
+  throw new Error("Function not implemented.");
+}


Remove these... see my note above.

ibacher · 2021-07-19T15:22:57Z

src/apps/concepts/components/EnhancedTableToolbar.tsx

@@ -70,7 +70,7 @@ export const EnhancedTableToolbar = (props: EnhancedTableToolbarProps) => {
      )}
      {numSelected > 0 ? (
        <>
-          {showAddConcepts ? null : (
+          {!showAddConcepts  ? null : (


This line should read:

{showAddConcepts ? null : (

We need to change the conditions under which showAddConcepts is true, not just negate it (if we keep this in, this will show the add concepts part where we don't want to do that, i.e., where the user didn't start from their own dictionary).

) Co-authored-by: Ian <ian_bacher@brown.edu>

ibacher · 2021-07-19T20:40:31Z

src/apps/dictionaries/redux/actions.ts

-          }
-        : concept
-    );
+  return async (dispatch: Function, getState: function) => {


This is the small typo it's complaining about:

Suggested change

return async (dispatch: Function, getState: function) => {

return async (dispatch: Function, getState: Function) => {

function is a keyword Function is a type, if that makes any sense an here (after the :) this is serving as a type annotation.

… into OCLOMRS-967

ibacher

LGTM! Thanks @jwnasambu!

jwnasambu · 2021-07-23T13:22:26Z

@ibacher thanks for your guidance you gave me a glimpse on how to work on a high level ticket and think out of the box.

jwnasambu marked this pull request as draft July 1, 2021 13:59

ibacher requested changes Jul 1, 2021

View reviewed changes

jwnasambu force-pushed the OCLOMRS-967 branch from d0654f4 to 5d61380 Compare July 1, 2021 20:57

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

dfb9116

jwnasambu force-pushed the OCLOMRS-967 branch from 559d12a to dfb9116 Compare July 6, 2021 09:26

ibacher and others added 4 commits July 16, 2021 10:06

Slightly faster Docker builds

a4d2239

Fix build warnings

445c90b

Don't fail if screenshots not created

5ab9916

This is meant to cover the case where unit tests fail

Update upload-artifact action

16097e8

ibacher reviewed Jul 17, 2021

View reviewed changes

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

869234c

ibacher reviewed Jul 19, 2021

View reviewed changes

jwamalwa and others added 8 commits July 19, 2021 18:58

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

31b0ddf

OCLOMRS-1014: Link Dictionary Manager with Google Analytics (openmrs#721

648617b

) Co-authored-by: Ian <ian_bacher@brown.edu>

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

5c0638a

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

0791c98

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

6615201

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

463b576

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

7765a33

Apply prettier formatting

64923e6

ibacher reviewed Jul 19, 2021

View reviewed changes

ibacher and others added 2 commits July 21, 2021 10:25

Attempt to fix CI build

c6cbeac

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

5846f77

jwnasambu marked this pull request as ready for review July 21, 2021 20:07

jwamalwa added 2 commits July 21, 2021 16:12

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

74725ed

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

94932cd

jwamalwa and others added 7 commits July 21, 2021 16:12

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

364b7d7

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

ba4124e

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

cb7dd88

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

80f8c0c

OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)

95f9432

Merge branch 'OCLOMRS-967' of github.com:jwnasambu/openmrs-ocl-client…

0d4802a

… into OCLOMRS-967

Restore accidentally deleted code

65f8860

ibacher approved these changes Jul 23, 2021

View reviewed changes

ibacher changed the title ~~OCLOMRS-967:Pick concepts from a dictionary (in addition to sources)~~ OCLOMRS-967: Pick concepts from a dictionary (in addition to sources) Jul 23, 2021

ibacher merged commit 6cd0198 into openmrs:master Jul 23, 2021

ibacher deleted the OCLOMRS-967 branch July 23, 2021 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCLOMRS-967: Pick concepts from a dictionary (in addition to sources) #712

OCLOMRS-967: Pick concepts from a dictionary (in addition to sources) #712

jwnasambu commented Jun 30, 2021

coveralls commented Jun 30, 2021 •

edited

Loading

ibacher left a comment

ibacher Jul 1, 2021

ibacher Jul 1, 2021

ibacher Jul 1, 2021

ibacher Jul 1, 2021

ibacher Jul 1, 2021

ibacher Jul 1, 2021

ibacher Jul 1, 2021

ibacher Jul 1, 2021

ibacher Jul 1, 2021

jwnasambu Jul 1, 2021

hadijahkyampeire commented Jul 6, 2021

jwnasambu commented Jul 6, 2021

hadijahkyampeire commented Jul 7, 2021

ibacher left a comment

ibacher Jul 17, 2021

ibacher Jul 17, 2021

ibacher Jul 17, 2021

ibacher Jul 19, 2021

ibacher Jul 19, 2021

ibacher Jul 19, 2021

ibacher left a comment

jwnasambu commented Jul 23, 2021

	const groupedConcepts = groupBy(concepts.sourceUrl),
	const groupedConcepts = groupBy(concepts.sourceUrl);

	return async (dispatch: Function, getState: function) => {
	return async (dispatch: Function, getState: Function) => {

OCLOMRS-967: Pick concepts from a dictionary (in addition to sources) #712

OCLOMRS-967: Pick concepts from a dictionary (in addition to sources) #712

Conversation

jwnasambu commented Jun 30, 2021

JIRA TICKET NAME:

Summary:

coveralls commented Jun 30, 2021 • edited Loading

ibacher 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

hadijahkyampeire commented Jul 6, 2021

jwnasambu commented Jul 6, 2021

hadijahkyampeire commented Jul 7, 2021

ibacher 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

ibacher left a comment

Choose a reason for hiding this comment

jwnasambu commented Jul 23, 2021

coveralls commented Jun 30, 2021 •

edited

Loading