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

VACMS-11115: Add VA Form flag Views. #11251

Merged
merged 23 commits into from
Oct 21, 2022
Merged

VACMS-11115: Add VA Form flag Views. #11251

merged 23 commits into from
Oct 21, 2022

Conversation

chri5tia
Copy link
Contributor

@chri5tia chri5tia commented Oct 19, 2022

Description

Closes #11115 , #11118, #11117, #11116 (no github magic if you list them like this)
Closes #11118
Closes #11117
Closes #11116

Adds 3 new views to the VA Forms content administration area:

  • Changed Filename
  • Changed Title
  • New and deleted forms

Testing done:

(Updated)

  • Views bulk operation clears flags
  • View shows flagged items according to link tab, page title and table caption
  • Title/Name filter
  • Moderation state filter
  • Section filter
  • Form administration filter shows (VACO, VBA, VHA, NCA, OCTO)
  • Edit links
  • Columns match style guide order
  • Custom text and rewrite tokens work: updated by, locked by, edit button

Screenshots

Screen Shot 2022-10-19 at 4 50 54 PM

QA steps

  • New view should appear in VA Forms area: https://cms-gcjxrl4huhyj4lipesbplzfbyy2zsjcs.ci.cms.va.gov/admin/content/va-forms
  • New view follows CMS Views style guide
  • Display flags with columns
    • Title
    • Edit button
    • Content type
    • Updated (timestamp and author)
    • Moderation state
    • Section
    • Lock
    • Form administration
  • Bulk action to clear flags on selected items and that the action matches the type of flags shown
  • Requires PM/DM review
  • Click on edit for one form on each tab, then go back (no need to save)
  • Ensure that the form specifies that it is now locked in the lock column, for each tab
  • Ensure that edited by shows author
  • Check that all exposed filter views work

Definition of Done

  • Documentation has been updated, if applicable.
  • Tests have been added if necessary.
  • Automated tests have passed.
  • Code Quality Tests have passed.
  • Acceptance Criteria in related issue are met.
  • Manual Code Review Approved.

Select Team for PR review

  • Platform CMS Team
  • Sitewide program
  • ⭐️ Sitewide CMS
  • ⭐️ Public websites
  • ⭐️ Facilities
  • ⭐️ User support

Is this PR blocked by another PR?

  • DO NOT MERGE

Does this PR need review from a Product Owner

  • Needs PO review

CMS user-facing annoucement

Is an announcement needed to let editors know of this change?

  • Yes, and it's written in issue ____ and queued for publication.
    • Merge and ping the UX writer so they are ready to publish after deployment
  • Yes, but it hasn't yet been written
    • Don't merge yet -- ping the UX writer to write and queue content
  • No announcement is needed for this code change.
    • Merge & carry on unburdened by announcements

@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 00:17 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 05:22 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 05:34 Destroyed
@chri5tia chri5tia changed the title VACMS 11115 changed filename form flag with vbo VACMS-11115: Changed filename form flag view Oct 19, 2022
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 05:58 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 06:17 Destroyed
@chri5tia chri5tia marked this pull request as ready for review October 19, 2022 17:08
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 17:08 Destroyed
@chri5tia
Copy link
Contributor Author

Thanks for the feedback @dsasser on #11118. Applying it here first. I need this PR to deploy first because it has the patch for VBO and flags.

Content type filter is not necessary
Removed

Missing title/form name exposed filter per views style guide
Added

VBO checkbox is in the far right column; should be far left column
Fixed

Content type column is not necessary
Removed

We should probably follow the existing practice employed by Facilities, which is to add all related product views to the same View as individual displays. See /admin/structure/views/view/facility_services/edit/content_audit_facilities?destination=/admin/content/facilities

All the form views are different displays of the "flagged" view, because they are basic duplicates of that view with small alterations. I'd rather keep it here for now, since backend changes to this should all be in the same place.

Column order does not follow views style guide.
Fixed

Missing moderation state filter per views style guide
Added

@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 18:44 Destroyed
@chri5tia chri5tia requested a review from dsasser October 19, 2022 18:54
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 19:01 Destroyed
@chri5tia chri5tia mentioned this pull request Oct 19, 2022
36 tasks
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 20:49 Destroyed
@chri5tia chri5tia mentioned this pull request Oct 19, 2022
12 tasks
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 20:57 Destroyed
@jilladams jilladams linked an issue Oct 19, 2022 that may be closed by this pull request
12 tasks
Copy link
Contributor

@swirtSJW swirtSJW left a comment

Choose a reason for hiding this comment

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

Noticed a couple of things

  • updated is missing the user reference

image

  • The edit button links are missing the node ids so the links don't function
    image

  • The locked column does not reflect that something is locked. Though to be honest, with how crowded the table it, we may want to consider abandoning the locked column in favor of readability
    image

  • The last "Changed Filename" view tab is actually a Deleted View.

    • The deleted should be included in the "New Form" View as that will surface the common issue of one being deleted and a duplicate created. I suggest changing the name of the View to "New/Deleted Forms"
  • Tables need captions. (Please add captions to our style guide doc, I think it is missing.)
    image

@swirtSJW swirtSJW changed the title VACMS-11115: Changed filename form flag view VACMS-11115: Add VA Form flag Views. Oct 19, 2022
@jilladams
Copy link
Contributor

jilladams commented Oct 19, 2022

In addition to Steve's note:

  • For Flags and Lock columns, the Table header and table columns are swapped (wrong header appears on the column).
    • Flags column header should appear over Flags field
    • Lock header should appear over Lock field, unless we remove it altogether. (I'm ok with removing.)
  • Can you add Form administration as a Filter on all 4 views?

#11118 https://pr11251-gcjxrl4huhyj4lipesbplzfbyy2zsjcs.ci.cms.va.gov/admin/content/va-forms/changed-title
#11115 https://pr11251-gcjxrl4huhyj4lipesbplzfbyy2zsjcs.ci.cms.va.gov/admin/content/va-forms/changed-filename
#11117 https://pr11251-gcjxrl4huhyj4lipesbplzfbyy2zsjcs.ci.cms.va.gov/admin/content/va-forms/new-form
#11116 https://pr11251-gcjxrl4huhyj4lipesbplzfbyy2zsjcs.ci.cms.va.gov/admin/content/va-forms/deleted-form

@chri5tia
Copy link
Contributor Author

chri5tia commented Oct 19, 2022

@swirtSJW @jilladams The above comments and concerns are fixed. I realized I broke some things when I rearranged columns for the style guide. Oops!

Edited PR body.

@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 19, 2022 23:57 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 20, 2022 00:08 Destroyed
@chri5tia chri5tia requested a review from swirtSJW October 20, 2022 00:09
Copy link
Contributor

@swirtSJW swirtSJW left a comment

Choose a reason for hiding this comment

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

@chri5tia This is looking really good. A couple things

  • Lets not surface the field name in the filter. Filed names are only for us drupal geeks and don't have much value to others, and create more confusion.
    image

  • The pattern of the actions on the other Views use the pattern of resolved
    Where the new deleted breaks that pattern
    image
    HOWEVER, I would also touch base quickly with @BlakeOrgan Since these are names for actions, I think they want to be phrased as an action, not a noun. Example: "Remove Deleted flag", "Remove Filename Changed flag"

  • This one is scope creep so consult with @jilladams and @wesrowe but I think I recall Dave suggesting this AND it is super easy to add. I suggest removing the locked column and adding the revision log message to ALL the View displays. I think it makes these tools even more useful than they already are.
    image

@chri5tia
Copy link
Contributor Author

chri5tia commented Oct 20, 2022

@swirtSJW machine name was an oversight, thank you! Regarding the other items:

  • I will consult regarding replacing lock with the change log. Wondering if that might be a part two?
  • Regarding the names of the items in the drop down, they were already called that and didn't want to change the name due to possibly being used in other places. I will consult but wondering if that would also be a good part two?

@dsasser
Copy link
Contributor

dsasser commented Oct 20, 2022

@chri5tia @swirtSJW quick note on the lock field in the table: lock changes are not immediately visible in the View. To replicate:

  • Find an unlocked node in the View
  • Edit the node in a new tab
  • Refresh the View
  • Notice the lock column doesn't change for the affected node
  • Perform a cache clear
  • Refresh the View
  • Notice the lock column is now correct

This may be expected/well known, but figured I would flag it up for visibility.

@swirtSJW
Copy link
Contributor

Regarding lock, if we want to keep it, we should remove tag caching on the View. I don't think the lock state is worth keeping.

@swirtSJW
Copy link
Contributor

swirtSJW commented Oct 20, 2022

@chri5tia I see what you mean about the actions. They are being pulled from the flag itself.
image

I think the wording of those should change, but does not have to be part of this issue. I am spinning up another ticket now to
#11285

@dsasser
Copy link
Contributor

dsasser commented Oct 20, 2022

Yeah I would agree we don't need the lock column and by removing it we can retain Views caching.

@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 21, 2022 21:51 Destroyed
@jilladams
Copy link
Contributor

Re-verified on https://pr11251-gcjxrl4huhyj4lipesbplzfbyy2zsjcs.ci.cms.va.gov/:

  • All 3 new tabs match page contents / flags
  • Edit links working on all 3 views
  • Filters working on all 3 views
  • Flag actions on New / Deleted are updated
  • Action to remove flag is working
  • Column order is consistent across all 3 views
  • Headers match table contents
  • Lock column is removed
  • Updated column shows username
  • Revision log message shows on all 3
  • Form admin filter doesn't show machine name
  • New/deleted is showing table data

Only thing I don't know how to verify is that all 3 tables have captions added. (Could someone show me that?)

Copy link
Contributor

@swirtSJW swirtSJW left a comment

Choose a reason for hiding this comment

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

Tests out good. Nice work.

I added a commit did a couple of barely noticeable things.

  1. I removed the lock releated fields from the table as they were now all hidden.
  2. I removed the label overrides so they they could be controlled by the content on the flag, rather than config in the override on the view. ALso it is a separate ticket needing design input Reword VA Form flag "Unflag link text" to describe the action. #11285
  3. Rearranged the filters to move moderation state left of section. Section and admin are somewhat paired so it makes sense to have them paired visually.

@swirtSJW
Copy link
Contributor

I think the php unit test is a fluke.
image

I am running it again

@swirtSJW swirtSJW enabled auto-merge (squash) October 21, 2022 22:12
@chri5tia
Copy link
Contributor Author

Yeah, it has to do with the database save point failing and using a previous save point. I don't think we need to worry about that.

1) tests\phpunit\Content\NodeLinkEnforcementFilterTest::testProcess with data set #5 ('<a href="node/230947230952309...st</a>')
Drupal\Core\Entity\EntityStorageException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist: ROLLBACK TO SAVEPOINT savepoint_1; Array
(
)

@swirtSJW swirtSJW merged commit 67585bc into department-of-veterans-affairs:main Oct 21, 2022
@chri5tia chri5tia deleted the VACMS-11115-Changed-Filename-Form-Flag-VBO branch November 7, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants