Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Activity state display for plans in Gantt and Time list views #7370

Merged
merged 36 commits into from
Jan 28, 2024

Conversation

shefalijoshi
Copy link
Contributor

@shefalijoshi shefalijoshi commented Jan 10, 2024

Closes #7369

Also see VIPEROMCT PR 205

Describe your changes:

Adds an inspector view to display a list of possible states for activities
Adds persistence of activity states
Adds selection of activity rows in time list and display of activity properties (like in the gantt view)

Testing notes:

The plan Json requires each activity to have an "id" in order to set it's activity state
By default filtering includes the 'name' and any information in a "properties" object for each activity. For example:

{
    "TEST_GROUP": [
        {
            "name": "Past event 1",
            "start": 1705397296000,
            "end": 1705442241000,
            "type": "TEST-GROUP",
            "color": "orange",
            "textColor": "white",
            "id": 1,
            "description": "First past event",
            "properties": {
                "location": "Ballroom"
            }
        }
    ]
}

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Has this been smoke tested?
  • Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
  • Have you associated a milestone with this PR? Note: leave blank if unsure.
  • Is this a breaking change to be called out in the release notes?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 77 lines in your changes are missing coverage. Please review.

Comparison is base (60e1eeb) 55.95% compared to head (f404116) 55.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7370      +/-   ##
==========================================
- Coverage   55.95%   55.91%   -0.05%     
==========================================
  Files         662      666       +4     
  Lines       26432    26513      +81     
  Branches     2574     2585      +11     
==========================================
+ Hits        14790    14824      +34     
- Misses      10928    10974      +46     
- Partials      714      715       +1     
Flag Coverage Δ *Carryforward flag
e2e-full 41.87% <ø> (-0.01%) ⬇️ Carriedforward from 60e1eeb
e2e-stable 59.99% <64.51%> (+0.06%) ⬆️
unit 48.88% <18.81%> (-0.03%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
src/api/objects/ObjectAPI.js 91.96% <100.00%> (ø)
...lugins/activityStates/activityStatesInterceptor.js 100.00% <100.00%> (ø)
...s/activityStates/createActivityStatesIdentifier.js 100.00% <100.00%> (ø)
src/plugins/plan/components/ActivityTimeline.vue 50.00% <ø> (+16.66%) ⬆️
src/plugins/plan/plugin.js 100.00% <100.00%> (ø)
src/plugins/timelist/TimelistComponent.vue 59.19% <ø> (-0.54%) ⬇️
...imelist/inspector/TimeListInspectorViewProvider.js 66.66% <ø> (ø)
src/plugins/plan/components/PlanView.vue 55.55% <60.00%> (-0.10%) ⬇️
src/ui/components/List/ListView.vue 37.93% <0.00%> (-1.36%) ⬇️
...plan/inspector/components/PlanActivityTimeView.vue 0.00% <0.00%> (ø)
... and 4 more

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60e1eeb...f404116. Read the comment docs.

Copy link

deploysentinel bot commented Jan 10, 2024

Current Playwright Test Results Summary

✅ 123 Passing - ❌ 2 Failing - ⚠️ 2 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 01/19/2024 11:09:50pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: 6def4c2

Started: 01/19/2024 11:06:34pm UTC

❌ Failures

📄   functional/plugins/flexibleLayout/flexibleLayout.e2e.spec.js • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Flexible Layout independent time works with flexible layouts and its children
Retry 2Retry 1Initial Attempt
Error: Console error detected: [error] Failed to load resource: the server responded wi...
Console error detected: [error] Failed to load resource: the server responded with a status of 404 () at (https://history.nasa.gov/alsj/a16/AS16-117-18744.jpg 0:0)

expect(received).not.toEqual(expected) // deep equality

Expected: not "error"

75% (6) 6 / 8 runs
failed over last 7 days
0% (0) 0 / 8 runs
flaked over last 7 days

📄   functional/clearDataAction.e2e.spec.js • 1 Failure

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Clear Data Action works as expected with Example Imagery
Retry 2Retry 1Initial Attempt
Error: Timed out 5000ms waiting for expect(locator).toBeVisible()...
Timed out 5000ms waiting for expect(locator).toBeVisible()

Locator: locator('.c-imagery__main-image__background-image')
Expected: visible
Received: hidden
Call log:
  - expect.toBeVisible with timeout 5000ms
  - waiting for locator('.c-imagery__main-image__background-image')
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"
  -   locator resolved to <div draggable="false" class="c-imagery__main-image__…></div>
  -   unexpected value "hidden"

75% (6) 6 / 8 runs
failed over last 7 days
0% (0) 0 / 8 runs
flaked over last 7 days

⚠️ Flakes

📄   functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Restricted Notebook with a page locked and with an embed @addinit Allows embeds to be deleted if page unlocked @addinit
Retry 1Initial Attempt
12.50% (1) 1 / 8 run
failed over last 7 days
62.50% (5) 5 / 8 runs
flaked over last 7 days

📄   functional/plugins/displayLayout/displayLayout.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Display Layout Toolbar Actions @localStorage can add/remove Image to a single layout
Retry 1Initial Attempt
0% (0) 0 / 8 runs
failed over last 7 days
12.50% (1) 1 / 8 run
flaked over last 7 days

View Detailed Build Results


@shefalijoshi shefalijoshi added this to the Target:3.3.0 milestone Jan 10, 2024
@shefalijoshi shefalijoshi requested review from jvigliotta, ozyx, michaelrogers and charlesh88 and removed request for jvigliotta and ozyx January 10, 2024 23:25
@unlikelyzero unlikelyzero modified the milestones: Target:3.3.0, Target:4.0.0 Jan 17, 2024
@ozyx ozyx self-requested a review January 18, 2024 00:11
@shefalijoshi shefalijoshi mentioned this pull request Jan 18, 2024
22 tasks
Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

  • Activity state should be settable from Gantt, and both compact and expanded Time List views.
  • We need a s-selected property to be applied to Activities in the Time List views in the same way that it is in the Gantt view.
  • The Set Activity State dropdown should use '- Set Status -' as its label, not '- Select Activity State -'.
  • When the state for an Activity hasn't been set, the Set Activity State dropdown needs selected to be applied to the '- Set Status -' option so it doesn't appear as a blank, like this:
    image
  • The current activity status needs to be displayed in the Inspector as 'Status' per the Figma mockup as on https://trunk.arc.nasa.gov/confluence/display/VIPERMS/Situational+Awareness+Needs+and+Design
  • Activities in Compact view should have a visual style when selected.
  • Same for Activities in Expanded view. Done in branch timelist-compact-view.

Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Halfway done reviewing. Just a few minor comments right now, but I have more to comment on with the PlanActivitiesInspectorView and the way we're passing props there. It's late though and I'll need time to collect my thoughts, but we really ought to restructure this.

I'm seeing this problematic pattern where we pass lists of objects into components that shouldn't maintain their own state, instead of passing props individually so Vue can pick and choose which DOM elements to render. This is a huge performance concern, and it's the very same pattern that we spent a lot of time refactoring out of PlotSeries. I left comments on the other PR regarding similar structural issues.

src/plugins/plan/util.js Show resolved Hide resolved
src/plugins/timelist/TimelistComponent.vue Show resolved Hide resolved
src/ui/components/List/ListView.vue Outdated Show resolved Hide resolved
src/ui/components/List/ListView.vue Outdated Show resolved Hide resolved
src/plugins/activityStates/plugin.js Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

super tiny change and then we're g2g

@shefalijoshi shefalijoshi enabled auto-merge (squash) January 25, 2024 21:04
Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

LGTM

@shefalijoshi shefalijoshi added the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 26, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 26, 2024
@ozyx ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 26, 2024
@ozyx ozyx disabled auto-merge January 26, 2024 22:12
@unlikelyzero unlikelyzero added pr:e2e:perf Trigger perf tests pr:e2e:couchdb npm run test:e2e:couchdb and removed pr:e2e:couchdb npm run test:e2e:couchdb labels Jan 26, 2024
@github-actions github-actions bot removed pr:e2e:couchdb npm run test:e2e:couchdb pr:e2e:perf Trigger perf tests labels Jan 26, 2024
@unlikelyzero unlikelyzero added the type:feature Feature. Required intentional design label Jan 28, 2024
@unlikelyzero unlikelyzero merged commit dc5a323 into master Jan 28, 2024
12 of 13 checks passed
@unlikelyzero unlikelyzero deleted the activity-state-display branch January 28, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature Feature. Required intentional design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting states for activities in a plan (gantt chart and time list views)
4 participants