[BI-563] Archive Individual Trait #70

dmeidlin · 2021-01-29T17:44:39Z

The Trait model was updated to include the "active" property and set it to true by default.

The Archiving features in the UI can be activated/deactivated by toggle the state of the archivable prop in TraitsListTable.
Archivable should be set to false until the "active" property is included in trait data from bi-api.

Given: The user has navigated to the "Trait Management" screen
And: the user has selected a trait to view
When: the user clicks "Archive" on the detail pane
Then: ensure that a confirmation dialog appears

Given: The user has clicked “Archive”
When: they confirm archiving the trait
Then: ensure that the trait detail pane is closed
And: ensure that the row shows an indicator that the trait is archived
And: ensure that a success notification is displayed at the top of the screen

Given: The user has clicked “Archive”
When: they decline archiving the trait
Then: ensure that the archive confirmation dialog closes
And: ensure that the trait is not archived

Given: The user has navigated to the "Trait Management" screen
When: the user has selected an archived trait to view
Then: ensure that the ‘Archived’ tag is displayed at the bottom of the detail pane

Given: that there is more than one page of traits in the trait list
When: the user archives a trait that is not on the first page
Then: ensure that the page in view is reset to the first page

ctucker3

Looks good! I didn't run it locally but the code looks good. I had a few questions and comments, but approving since I'll be out the rest of the week.

ctucker3 · 2021-02-02T18:00:22Z

package.json

@@ -73,7 +73,7 @@
    "vue-cli-plugin-axios": "0.0.4",
    "vue-template-compiler": "^2.6.10"
  },
-  "devport": "8080",
+  "devport": "8082",


Does this need to stay changed or can we change it back to 8080?

The webserver is running on 8082 because the brapi-server is running on 8080.
There are other PRs related to this pending review:
Breeding-Insight/bi-docker-stack#8,
Breeding-Insight/bi-api#67

The brapi server runs internally in the container as 8080 and externally as 8083, so I think the web could stay as 8080.

Ok, will change the port back.

ctucker3 · 2021-02-02T18:39:49Z

src/components/trait/TraitDetailPanel.vue

@@ -70,6 +70,17 @@
      <p class="has-text-weight-bold mt-3 mb-0">Description of collection method</p>
      <p>{{data.method.description}}</p>

+      <template v-if="archivable && !data.active">


Can this just be !data.active? The data can be archived without the table providing archiving abilities. One route I can think of is if the status is set to inactive in the import (not sure if we support that yet).

Yes, the !data.active check is all that is required once the backend returns the active status as part of the trait data.

When the traits are pulled from the backend the trait data is looped over all defined key values when creating the trait objects for the frontend. Since the active property does not appear in the trait data, data.inactive will be undefined and !data.active will be true for all traits even though they are active. So the achivable check is in place to turn these sections off until the backend can support archiving. Once the backend part is working the archivable check can be removed.

ctucker3 · 2021-02-02T18:40:49Z

src/components/trait/TraitListsTable.vue

+            class="button is-danger"
+            v-on:click="modalDeleteHandler"
+          >
+            <strong>Yes, {{ focusTrait.active ? 'remove' : 'restore' }}</strong>


Should this be Yes, archive instead of Yes, remove?

I used "remove" because it was already in place in the warning modal. The following is from commit ee46948:
`

Program-related data referencing this trait will not be affected by this change.

Yes, remove Cancel

` The design reference doesn't show anything for the warning modal, and I think "archive" is more accurate than "remove", so I'm fine with making the change.

Sorry the code didn't typeset well; try again...

Program-related data referencing this trait will not be affected by this change.

Yes, remove
Cancel

Ok, this works. Sorry for spamming your inbox. :-)

<WarningModal v-bind:active.sync="deactivateActive" v-bind:msg-title="'Remove trait from Program name?'" v-on:deactivate="deactivateActive = false" > <section> <p class="has-text-dark"> Program-related data referencing this trait will not be affected by this change. </p> </section> <div class="columns"> <div class="column is-whole has-text-centered buttons"> <button v-on:click="modalDeleteHandler()" class="button is-danger"><strong>Yes, remove</strong></button> <button v-on:click="deactivateActive = false" class="button">Cancel</button> </div> </div> </WarningModal>

ctucker3 · 2021-02-02T18:43:28Z

src/components/trait/TraitListsTable.vue

+
+  async modalDeleteHandler(){
+    try {
+      const traitClone = JSON.parse(JSON.stringify(this.focusTrait));


Never knew that was a way to clone! There is also the Trait.assign function in the trait object if you want a trait class instance.

Assign would probably work in this case, but I generally stay away from it because it can cause errors if a property of interest is deeply nested (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign).

Also, I remember seeing a comparison of computational effort required for the JSON parsing method for deep cloning versus using methods in underscore or other libraries, and JSON parsing came out on top, so I generally use it for cloning.

That makes sense. Just to clarify, the Trait.assign is a custom function of ours that is implemented in our classes.

nickpalladino · 2021-02-03T21:44:22Z

src/components/trait/TraitListsTable.vue

@@ -117,10 +134,13 @@
          v-bind:edit-active="traitSidePanelState.editActive"
          v-bind:edit-btn-active="editFormBtnActive"
          v-bind:editable="true"
+          v-bind:archivable="false"


Why is this false? I guess that's why I don't see an archive link in the detail pane?

Just documenting what we talked about earlier:
The frontend logic works once the "active" trait is returned from the api. Mocking out flagging some traits as archived and others not while preventing the mocks to be overwritten by calls to the trait api was beyond the scope of this work and not really of great value. Without the archivable boolean flag stored per trait the feature can't function properly, so the archivable flag is set to false to turn the feature off until the backend is ready.

Can we fix bi-api as part of this card so that we can test this feature? It looks like createTraits in TraitService needs to set the active field in the TraitEntity as it's writing null to the database currently.

For traceability, I fixed bi-api in the BI-732 PR that is currently open.

nickpalladino · 2021-02-03T21:47:32Z

src/components/trait/TraitListsTable.vue

+            class="button is-danger"
+            v-on:click="modalDeleteHandler"
+          >
+            <strong>Yes, {{ focusTrait.active ? 'remove' : 'restore' }}</strong>


nickpalladino · 2021-02-03T21:48:45Z

src/components/trait/TraitListsTable.vue

@@ -245,6 +267,30 @@ export default class TraitTable extends Vue {
    });
  }

+  activateArchive(focusTrait: Trait){
+    if (focusTrait.active) {
+      this.deactivateWarningTitle = `Remove "${focusTrait.traitName}" from ${this.activeProgram!.name!}?`;


Similar to previous comment, maybe this should also use archive terminology

ctucker3

From our discussion during sprint planning. Can we move the Archive tag to another column or create a new card to do that?

DebWeigand

This scenario, as is written in the card, does not pass (unless I am misunderstanding)
Given: that there is more than one page of traits in the trait list
When: the user archives a trait that is not on the first page
Then: ensure that the page in view is reset to the first page

after archiving the trait on the second page, the view does not go back to the first page. BUT, do we really want it too? Don't we want to see it in the list as archived on the original page it was on? Or, I may be just misunderstanding...

dmeidlin · 2021-02-19T21:10:01Z

@ctucker3 I think it's fine adding the archive badge as another column in this PR. Now that the backend is working I'll merge in the updates from #77 and add the change with the extra column.

…etailPanel

…g archive status

github-actions bot added the feature label Jan 29, 2021

dmeidlin requested review from ctucker3 and nickpalladino January 29, 2021 17:45

ctucker3 approved these changes Feb 2, 2021

View reviewed changes

nickpalladino requested a review from DebWeigand February 3, 2021 21:10

nickpalladino reviewed Feb 3, 2021

View reviewed changes

dmeidlin force-pushed the feature/BI-563 branch from e7f0418 to 81bfd7a Compare February 9, 2021 17:05

ctucker3 suggested changes Feb 11, 2021

View reviewed changes

DebWeigand suggested changes Feb 17, 2021

View reviewed changes

dmeidlin force-pushed the feature/BI-563 branch from c129b98 to 50e2c76 Compare February 23, 2021 20:09

dmeidlin requested review from ctucker3, nickpalladino and DebWeigand February 24, 2021 18:23

ctucker3 approved these changes Feb 25, 2021

View reviewed changes

DebWeigand approved these changes Feb 25, 2021

View reviewed changes

nickpalladino approved these changes Feb 25, 2021

View reviewed changes

dmeidlin added 12 commits March 1, 2021 11:50

[BI-563-1] make dev port for bi-web unique

d6f6c31

[BI-563-1] activate WarningModal when archive event fired from TraitD…

8da53ca

…etailPanel

[BI-563-1] display active program name in warning modal for archive

ad0f65e

[BI-563-1] close details pane when archive confirmed

7708d2b

[BI-563-1] add restore/unarchive link to detail panel

163b205

[BI-563-1] go to page 1 of trait list after confrming archive

a1fafaa

[BI-563-1] show success notification after trait archiving

349f760

[BI-563-1] add archive tag to to bottom of trait detail panel

263819a

[BI-563-1] show archived tag on trait table row

a75bb07

[BI-563-1] fix check for absence of trait id in trait service

2f47011

[BI-563-1] update traits on backend when archiving

0859784

[BI-563-1] substitute remove/restore in user prompts for correspondin…

4977594

…g archive status

dmeidlin and others added 8 commits March 1, 2021 11:50

[BI-563] use archivable flag

6e6d619

[BI-563] fix input type

dc07223

[BI-563] fix type issue

d5213b3

[BI-563] change verbage remove to archive

e95a7fb

[BI-732] finish up archive traits on front end

3714efa

[BI-563] remove unnecessary archivable flag check

6d2a5eb

[BI-563] change dev port to 8080

889debb

[BI-563] return to first page after archiving trait

edaa341

dmeidlin force-pushed the feature/BI-563 branch from 75b54f5 to edaa341 Compare March 1, 2021 16:52

dmeidlin merged commit 061aae7 into develop Mar 1, 2021

dmeidlin deleted the feature/BI-563 branch March 1, 2021 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BI-563] Archive Individual Trait #70

[BI-563] Archive Individual Trait #70

dmeidlin commented Jan 29, 2021

ctucker3 left a comment

ctucker3 Feb 2, 2021

nickpalladino Feb 3, 2021

dmeidlin Feb 8, 2021

ctucker3 Feb 11, 2021

dmeidlin Feb 19, 2021

ctucker3 Feb 2, 2021

dmeidlin Feb 8, 2021

ctucker3 Feb 2, 2021

nickpalladino Feb 3, 2021

dmeidlin Feb 8, 2021

dmeidlin Feb 8, 2021

dmeidlin Feb 8, 2021

ctucker3 Feb 2, 2021

dmeidlin Feb 8, 2021

ctucker3 Feb 11, 2021

nickpalladino Feb 3, 2021

dmeidlin Feb 8, 2021

nickpalladino Feb 9, 2021

ctucker3 Feb 15, 2021

nickpalladino Feb 3, 2021

nickpalladino Feb 3, 2021

dmeidlin Feb 8, 2021

ctucker3 left a comment

DebWeigand left a comment

dmeidlin commented Feb 19, 2021

[BI-563] Archive Individual Trait #70

[BI-563] Archive Individual Trait #70

Conversation

dmeidlin commented Jan 29, 2021

ctucker3 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

ctucker3 left a comment

Choose a reason for hiding this comment

DebWeigand left a comment

Choose a reason for hiding this comment

dmeidlin commented Feb 19, 2021