[PM-10426] Admin Console - Edit Modal

[nick-livefront]

🎟️ Tracking

PM-10426

📔 Objective

• Add the Extension Refresh styling to the Admin Console Vault
  • Integrated with the combined modal work done in [PM-12389] Combined web vault item dialog #11345
• Added a config option and relevant UI to hide the folder selection in the Add/Edit modal. Folders aren't applicable to admin console.

@shane-melton @danielleflinn You can see in my videos below that the vault doesn't update until the dialog is closed. Should I add functionality to refresh the vault when a cipher is saved and the user is navigated to the view dialog?

📸 Screenshots

Add Cipher	Edit Cipher
ac-add-cipher.mov	ac-edit-cipher.mov

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[codecov]

Codecov Report

Attention: Patch coverage is 42.04545% with 51 lines in your changes missing coverage. Please review.

Project coverage is 33.28%. Comparing base (d7d7426) to head (f04fef5).
Report is 15 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...pps/web/src/app/vault/org-vault/vault.component.ts	0.00%	45 Missing ⚠️
...rvices/admin-console-cipher-form-config.service.ts	86.04%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11249      +/-   ##
==========================================
+ Coverage   33.26%   33.28%   +0.02%     
==========================================
  Files        2729     2731       +2     
  Lines       85524    85593      +69     
  Branches    16314    16325      +11     
==========================================
+ Hits        28449    28491      +42     
- Misses      54816    54840      +24     
- Partials     2259     2262       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Checkmarx One – Scan Summary & Details – 964fd8ca-0d27-4e8c-9d71-231a0982032b

No New Or Fixed Issues Found

[danielleflinn]

Screen recordings look good as I expect the generator dialogs will be updated later when the generator PR is merged.

Should I add functionality to refresh the vault when a cipher is saved and the user is navigated to the view dialog?

Hmm, I think it would make sense to refresh the Vault on Save of a new item, if it isn't too complex. But I will differ to Shane on the final call, ideally the behavior would be consistent with what we have in the end user PM view.

[shane-melton]

Nice work on this! I think its coming together nicely! I just see a few permission issues we'll need to resolve.

As for refreshing the Vault after a cipher is saved, but before closing the dialog; we can find a way to do that, but I had left it to only refresh on dialog close in the PM. Watching for the dialog result was the most straightforward way to know if a save event occured.

If we want to refresh on every save, we'll need to include some type of event emitter in our dialogRef so that the caller can listen to both the close and save observables.

[shane-melton]

⚠️ This will fail for Unassigned ciphers as they're not available in the cipherService. We were passing in the entire cipherView to this method previously to avoid needing to repeat the logic performed in building the allCiphers$ observable.

I think we should still pass the view object in here directly, and if it doesn't exist we should return early as it is not possible to open the dialog in View mode without a cipher.

[nick-livefront]

Great thought. ca3a369

I think the edit would need the same thing, no? editCipherId follows this pattern. Any reason it isn't necessary there?

(Maybe it is, I'm having a difficult time getting the correct scenario setup to test)

[shane-melton]

Ah, good catch, it will also fail there as well and we should make the same change there.

For clarity, "fail" means it will simply skip the re-prompt and continue to the edit dialog. The edit dialog opened successfully because it has custom logic internally to retrieve the cipher from the admin API if its not available in the cipherService. That's also why this has likely gone un-noticed, you would specifically need to have MP re-prompt enabled for an unassigned item that is opened in the AC.

(btw, you can create unassigned items by deleting the last collection an item is assigned to)

[nick-livefront]

👍 Cool, I shadowed the same view logic for the edit logic: 87ff20a

[shane-melton]

Just two more small nits/suggestions and the change to pass CipherView to editCipher() and then I think this is good to go! Nice work!

[shane-melton]

I think this looks good to go!

We should likely do a quick test/pass without the feature flag enabled to ensure we're not breaking anything with the changes related to passing in the full cipher objects. (or simply wait until after the next rc cut next week)