OCLOMRS-913: The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary #723

jwnasambu · 2021-07-26T14:48:56Z

JIRA TICKET NAME:

The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary

Summary:

Created an automated test for "Add a few concepts from CIEL in existing dictionary"

ibacher

@jwnasambu I've added some comments here. Thanks for your hard work on this. I hope these comments are enough to get you unblocked!

ibacher · 2021-07-26T19:03:52Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+  cy.visit(`/users/${getUser()}/collections/${getDictionaryId()}/concepts/`);
+});
+
+When("the user clicks the Add concepts button", () => {


So, when I look at the build log for this, I see that the reason it is failing is this: Error: Step implementation missing for: the user clicks the "Add concepts" button. There are a couple of others like that.

In the feature file, the step is written as: When the user clicks the "Add concepts" button. Note the quotation marks around Add concepts those need to be here two, so you can either write this as:

When('the user clicks the "Add concepts" button')

Or:

When("the user clicks the \"Add concepts\" button")

To get the same result. Similarly with other tests where you have quotation marks.

ibacher · 2021-07-26T19:04:48Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+});
+
+Then("the user should be on the Import existing concept page", () => {
+  cy.visit("/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/openmrs/collections/")


This should be something like cy.url().should("contain", "<url>"). cy.url() gets the current URL. cy.visit() redirects the user to the provided url.

Also, instead of hard-coding openmrs as the user here, we should use ${getUser()}. Note that to support this the string should be surrounded by back-ticks `. (The difference between normal strings defined with ' or " and strings defined with ` is that the strings defined with ` allow this kind of inline string interpolation.

ibacher · 2021-07-26T19:05:55Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+});
+
+Given("the user is on the Import existing concept page", () => {
+  cy.visit("/orgs/CIEL/sources/CIEL/concepts/?addToDictionary")


We should fill in the addToDictionary here, e.g.,

Suggested change

cy.visit("/orgs/CIEL/sources/CIEL/concepts/?addToDictionary")

cy.visit(`/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/${getUser()}/collections/${getDictionaryId()}/`)

ibacher · 2021-07-26T19:09:02Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+});
+
+Then("the Serum concept should be added to the dictionary", () => {
+  cy.visit("/orgs/CIEL/sources/concepts/?addToDictionary=%2Fusers%2Fopenmrs%2Fcollections%2Fjst12%2F&page=1&q=Serum/")


Here we should actually be testing the dictionary contents right?

jwnasambu · 2021-07-26T19:14:01Z

@ibacher thanks for the review and the proposed changes.

jayasanka-sack · 2021-07-29T14:17:33Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+});
+
+Then('the "Serum" concept should be added to the dictionary', () => {
+  cy.visit(`/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/${getUser()}/collections/${getDictionaryId()}/Serum/`)


Isn't it should be something like this?

Suggested change

cy.visit(`/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/${getUser()}/collections/${getDictionaryId()}/Serum/`)

cy.url().should('include', `/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/${getUser()}/collections/${getDictionaryId()}/Serum/`)

Check the following:
https://docs.cypress.io/api/commands/url#Assert-the-URL-is-http-localhost-8000-users-1-edit

Is there any other way to determine whether the concept has been added? instead of checking the URL? A user-specific way.

ex: How a user knows when it has been created? Is it displays a success message? or it displays the created concept?

Since for this case am importing the concept from the already existing concepts, it just displays the imported concept in the dictionary.

jayasanka-sack · 2021-07-29T14:18:31Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+});
+
+Given('the user is on the "Import existing concept" page', () => {
+  cy.url().should("contain", "/orgs/CIEL/sources/CIEL/concepts/");


I guess this one should be

Suggested change

cy.url().should("contain", "/orgs/CIEL/sources/CIEL/concepts/");

cy.visit("/orgs/CIEL/sources/CIEL/concepts/");

…cepts from CIEL in existing dictionary

jayasanka-sack · 2021-08-02T16:45:08Z

cypress/integration/dictionary/addConceptInDictionaryPage.feature

+    Given the user is on the view dictionary concepts page
+     When the user clicks the "Add concepts" button
+      And the user selects "Import existing concept"
+      And the user selects "Pick concepts"


Seems the step implementation missing for this one

jayasanka-sack · 2021-08-02T16:47:54Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+});
+
+When('the user clicks on the row for "Serum"', () => {
+  cy.findByText("Serum").click();


cypress throws the following error,

Unable to find an element with the text: Serum. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

as described in the error message, it seems there's no such element with the text "Serum"

Yes, to address this, we actually need to create a concept called "Serum" in a source called "CIEL" owned by an organisation called "CIEL". These tests are run against an empty instance of OCL (i.e., OCL without any content).

This will probably require a new command (since the createSource command only supports created a user source).

So we need something called before this function that does something like this:

cy.createOrganisation("CIEL", true).createOrgSource("CIEL", "CIEL", true);

And then afterwards,

cy.deleteOrganisation("CIEL", true);

Will also need a createConcept command that takes the source that the concept belongs to and the minimum number of properties we need so we can create a "Serum" concept before this test. Does that make some kind of sense?

jayasanka-sack · 2021-08-02T16:49:42Z

...support/step_definitions/dictionary/addConceptInDictionaryPage/addconceptInDictioanryPage.ts

+  cy.findByTitle("Add concepts").click();
+});
+
+Then('the user selects "Import existing concept"', () => {


This on should be When not Then

Suggested change

Then('the user selects "Import existing concept"', () => {

When('the user selects "Import existing concept"', () => {

cypress/integration/dictionary/addConceptInDictionaryPage.feature

ibacher · 2021-08-02T18:56:08Z

@jwnasambu Please see @jayasanka-sack's helpful comments and let me know if there's any additional guidance you need.

jwnasambu · 2021-08-02T19:04:42Z

@ibacher and @jayasanka-sack for the guidance. Ian, am soon reaching out.

…cepts from CIEL in existing dictionary

jwnasambu · 2021-08-06T06:57:57Z

@suruchee I will be the happiest person to change the of status this PR but us you can see the build is still failing and basing on Ian's explanation Oh… I kind of see what the issue is. The loading of CIEL concepts is dependent on them not existing (since, in the environment that we run them in for CI, they don’t exist), but if you run it against staging, as you’re doing here, they’ll fail because CIEL already exists and has those concepts (and the user we login with doesn’t have permission to create CIEL concepts). The way to fix that is probably just to check if the CIEL Org, Source and concepts exist and only try to create them if they don’t. We have similar checks like that for some of the other create commands. I can work on that tomorrow, but I’m kind of busy the rest of this evening. Once the issue is fixed , the PR status will definitely change. Thanks in advance.

jwnasambu · 2021-08-06T13:35:12Z

@jayasanka-sack am sorry I had not your comment. Kindly forgive me.

…cepts from CIEL in existing dictionary

jwnasambu · 2021-08-09T13:08:22Z

@jwnasambu The element you trying to click has CSS pointer-events: none. That means the element is not clickable.

Try clicking on a clickable element, or you can use {force: true} to click it forcefully (not a good idea, btw).

@jayasanka-sack Am still getting the same error.

jayasanka-sack · 2021-08-09T16:13:04Z

cypress/support/step_definitions/dictionary/addConceptsToDictionary/addConceptsToDictionary.ts

+  cy.visit("users/openmrs/collections");
+});
+
+When('the user clicks on the "Add selected to dictionary" button', () => {


Suggested change

When('the user clicks on the "Add selected to dictionary" button', () => {

When('the user clicks the "Add selected to dictionary" button', () => {

jayasanka-sack · 2021-08-09T16:14:11Z

cypress/support/step_definitions/dictionary/addConceptsToDictionary/addConceptsToDictionary.ts

+    .click();
+});
+
+When('the user clicks the "Add Serum to dictionary" button', () => {


Suggested change

When('the user clicks the "Add Serum to dictionary" button', () => {

When('the user clicks on the "Add Serum to dictionary" button', () => {

@jayasanka-sack and @ibacher it has be a tough journey but am glad am remaining with 1 error though the error on the PR is different from the one on the terminal.

…cepts from CIEL in existing dictionary

coveralls · 2021-08-10T18:49:36Z

Coverage increased (+0.05%) to 46.768% when pulling 0085357 on jwnasambu:OCLOMRS-913 into 2a29e87 on openmrs:master.

ibacher · 2021-08-10T18:52:29Z

@hadijahkyampeire If you have a chance, could you take a look at the changes to the ViewConceptsPage.tsx and let me know if any of the changes there don't make sense?

jwnasambu · 2021-08-10T18:53:18Z

@ibacher and @jayasanka-sack I can't thank you enough. It has been a real fight I now have a glimpse of what it means to be a senior dev. Though with serious errors!

hadijahkyampeire · 2021-08-10T18:54:41Z

@hadijahkyampeire If you have a chance, could you take a look at the changes to the ViewConceptsPage.tsx and let me know if any of the changes there don't make sense?

Okay @ibacher I will do that.

hadijahkyampeire

I think all is fine, they all make sense and will make the importing from another dictionary work well, great work @ibacher and @jwnasambu . LGTM

hadijahkyampeire · 2021-08-10T18:57:50Z

src/apps/concepts/pages/ViewConceptsPage.tsx

@@ -336,7 +334,10 @@ const ViewConceptsPage: React.FC<Props> = ({
                )
              }
              dictionaryToAddTo={dictionaryToAddTo}
-              linkedDictionary={containerUrl}
+              linkedDictionary={
+                (containerType === DICTIONARY_CONTAINER && containerUrl) ||


Nice ❤️

jwnasambu marked this pull request as draft July 26, 2021 14:50

ibacher reviewed Jul 26, 2021

View reviewed changes

jwnasambu force-pushed the OCLOMRS-913 branch from 738667a to 5026f28 Compare July 28, 2021 10:06

jayasanka-sack reviewed Jul 29, 2021

View reviewed changes

jwnasambu force-pushed the OCLOMRS-913 branch 2 times, most recently from 40ed79f to b9aa234 Compare July 29, 2021 18:50

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

cc7ccd6

…cepts from CIEL in existing dictionary

jwnasambu force-pushed the OCLOMRS-913 branch from fee6690 to cc7ccd6 Compare July 29, 2021 19:35

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

8f9b01e

…cepts from CIEL in existing dictionary

jayasanka-sack reviewed Aug 2, 2021

View reviewed changes

cypress/integration/dictionary/addConceptInDictionaryPage.feature Outdated Show resolved Hide resolved

ibacher and others added 13 commits August 2, 2021 16:41

Add some default concepts for CIEL and fix a few things to work better

2c8a055

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

dc81149

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

1899f74

…cepts from CIEL in existing dictionary

Add check for Serum concept in dictionary

c446314

Click next to the link rather than on the link

b0a3049

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

8183171

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

350dc92

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

36bbc49

…cepts from CIEL in existing dictionary

Add some default concepts for CIEL and fix a few things to work better

51977a7

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

2e00b00

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

7c28e61

…cepts from CIEL in existing dictionary

Add check for Serum concept in dictionary

1f551a4

Click next to the link rather than on the link

4fe3ef4

Add some preflight checks to some potentially destructive operations

2c97361

ibacher and others added 2 commits August 6, 2021 09:53

Slight tweak

68d2592

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

9349c22

…cepts from CIEL in existing dictionary

jayasanka-sack reviewed Aug 9, 2021

View reviewed changes

jwamalwa and others added 8 commits August 9, 2021 19:22

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

7086be6

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

11ef131

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

7cb1ccb

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

3934af6

…cepts from CIEL in existing dictionary

OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few con…

6d0861d

…cepts from CIEL in existing dictionary

Fix the target tests

8e9f1e9

Wait until the dictionary is actually loaded before proceeding

0f4307d

Smarter initial load logic

cec3c10

ibacher marked this pull request as ready for review August 10, 2021 17:15

ibacher force-pushed the OCLOMRS-913 branch from cec3c10 to 0f4307d Compare August 10, 2021 17:30

ibacher added 2 commits August 10, 2021 14:18

Fix some URLs and the last test

9bfe90d

Proper clean-up to temporary organisation

0085357

ibacher approved these changes Aug 10, 2021

View reviewed changes

ibacher requested a review from hadijahkyampeire August 10, 2021 18:50

hadijahkyampeire approved these changes Aug 10, 2021

View reviewed changes

ibacher changed the title ~~OCLOMRS-913:The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary~~ OCLOMRS-913: The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary Aug 10, 2021

ibacher merged commit 1375bce into openmrs:master Aug 10, 2021

ibacher deleted the OCLOMRS-913 branch August 10, 2021 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCLOMRS-913: The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary #723

OCLOMRS-913: The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary #723

jwnasambu commented Jul 26, 2021 •

edited

Loading

ibacher left a comment

ibacher Jul 26, 2021

ibacher Jul 26, 2021

ibacher Jul 26, 2021

ibacher Jul 26, 2021

ibacher Jul 26, 2021

jwnasambu commented Jul 26, 2021

jayasanka-sack Jul 29, 2021

jayasanka-sack Jul 29, 2021

jayasanka-sack Jul 29, 2021

jwnasambu Jul 29, 2021

jayasanka-sack Jul 29, 2021

jayasanka-sack Aug 2, 2021

jayasanka-sack Aug 2, 2021

ibacher Aug 2, 2021

jayasanka-sack Aug 2, 2021

ibacher commented Aug 2, 2021

jwnasambu commented Aug 2, 2021

jwnasambu commented Aug 6, 2021 •

edited

Loading

jwnasambu commented Aug 6, 2021

jwnasambu commented Aug 9, 2021 •

edited

Loading

jayasanka-sack Aug 9, 2021

jayasanka-sack Aug 9, 2021

jwnasambu Aug 9, 2021

coveralls commented Aug 10, 2021

ibacher commented Aug 10, 2021

jwnasambu commented Aug 10, 2021

hadijahkyampeire commented Aug 10, 2021

hadijahkyampeire left a comment

hadijahkyampeire Aug 10, 2021

	cy.visit("/orgs/CIEL/sources/CIEL/concepts/?addToDictionary")
	cy.visit(`/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/${getUser()}/collections/${getDictionaryId()}/`)

	cy.visit(`/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/${getUser()}/collections/${getDictionaryId()}/Serum/`)
	cy.url().should('include', `/orgs/CIEL/sources/CIEL/concepts/?addToDictionary=/users/${getUser()}/collections/${getDictionaryId()}/Serum/`)

	cy.url().should("contain", "/orgs/CIEL/sources/CIEL/concepts/");
	cy.visit("/orgs/CIEL/sources/CIEL/concepts/");

	Then('the user selects "Import existing concept"', () => {
	When('the user selects "Import existing concept"', () => {

	When('the user clicks on the "Add selected to dictionary" button', () => {
	When('the user clicks the "Add selected to dictionary" button', () => {

	When('the user clicks the "Add Serum to dictionary" button', () => {
	When('the user clicks on the "Add Serum to dictionary" button', () => {

OCLOMRS-913: The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary #723

OCLOMRS-913: The Basic User: AddConceptInDictionaryPage: Add a few concepts from CIEL in existing dictionary #723

Conversation

jwnasambu commented Jul 26, 2021 • edited Loading

JIRA TICKET NAME:

Summary:

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

jwnasambu commented Jul 26, 2021

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

ibacher commented Aug 2, 2021

jwnasambu commented Aug 2, 2021

jwnasambu commented Aug 6, 2021 • edited Loading

jwnasambu commented Aug 6, 2021

jwnasambu commented Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 10, 2021

ibacher commented Aug 10, 2021

jwnasambu commented Aug 10, 2021

hadijahkyampeire commented Aug 10, 2021

hadijahkyampeire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwnasambu commented Jul 26, 2021 •

edited

Loading

jwnasambu commented Aug 6, 2021 •

edited

Loading

jwnasambu commented Aug 9, 2021 •

edited

Loading