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

Disable manual topic editing in the UI for GitHub App entries #1859

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

kathy-t
Copy link
Contributor

@kathy-t kathy-t commented Oct 24, 2023

Description
This PR goes with dockstore/dockstore#5713 and disables manual topic editing in the UI for GitHub app entries since they should be updating it in the .dockstore.yml instead. Topic selection is left as is, so the user can still change the selection for a GitHub app entry.

Also updated some tooltips for the topics.

Review Instructions
In addition to the instructions in dockstore/dockstore#5713, verify that the Edit button for editing manual topics is disabled for GitHub App entries.

Issue
dockstore/dockstore#5625
DOCK-2444

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that your code compiles by running npm run build
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If this is the first time you're submitting a PR or even if you just need a refresher, consider reviewing our style guide
  • Do not bypass Angular sanitization (bypassSecurityTrustHtml, etc.), or justify why you need to do so
  • If displaying markdown, use the markdown-wrapper component, which does extra sanitization
  • Do not use cookies, although this may change in the future
  • Run npm audit and ensure you are not introducing new vulnerabilities
  • Do due diligence on new 3rd party libraries, checking for CVEs
  • Don't allow user-uploaded images to be served from the Dockstore domain
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.
  • Check whether this PR disables tests. If it legitimately needs to disable a test, create a new ticket to re-enable it in a specific milestone.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ca76a6b) 40.53% compared to head (8e6d806) 40.47%.
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1859      +/-   ##
===========================================
- Coverage    40.53%   40.47%   -0.06%     
===========================================
  Files          365      366       +1     
  Lines        11283    11359      +76     
  Branches      2896     2913      +17     
===========================================
+ Hits          4574     4598      +24     
- Misses        4407     4448      +41     
- Partials      2302     2313      +11     

see 12 files with indirect coverage changes

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

<span
><strong
matTooltip="Short description of the {{ entryType$ | async }}, entered manually in the {{
workflow?.mode === WorkflowType.ModeEnum.DOCKSTOREYML ? '.dockstore.yml' : 'UI'
Copy link
Contributor

Choose a reason for hiding this comment

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

This message might be a little confusing for .dockstore.yml users that have previous entered a topic in the UI, because it'll tell them that it was entered in the .dockstore.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting you bring that up, I suppose this feature should come with some type of notification to .dockstore.yml users that they should modify their .dockstore.yml to include topics that they might have added in the UI. The webservice PR dockstore/dockstore#5713 won't overwrite existing topicManual if they don't do this so we won't lose it, but they should know about it

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I find tooltips are good to have, but half the users don't know to look for them (I often forget myself on other websites), and they shouldn't be relied as the sole way of conveying key info. Could you convey the info some other way as well? Ideas:

  • Next to/below the Topic Manual label, have some text saying the text must be edited in the .dockstore.yml.
  • When the user clicks the Edit button, a dialog pops saying edit the .dockstore.yml. I'm not 100% wild about this, but this is a brainstorm.

These are suggestions, not blocking, and it could stay the way it is as I suspect most users won't run into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I find tooltips are good to have, but half the users don't know to look for them (I often forget myself on other websites), and they shouldn't be relied as the sole way of conveying key info

I lean towards keeping it as is because this pattern exists for the other fields on the info tab. If we decide to re-design how to display tooltip info, we can migrate them all at once so the UI is consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This message might be a little confusing for .dockstore.yml users that have previous entered a topic in the UI, because it'll tell them that it was entered in the .dockstore.yml.

I specified in the disabled Edit button for GitHub app entries that editing in the UI is deprecated

<span
><strong
matTooltip="Short description of the {{ entryType$ | async }}, entered manually in the {{
workflow?.mode === WorkflowType.ModeEnum.DOCKSTOREYML ? '.dockstore.yml' : 'UI'
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I find tooltips are good to have, but half the users don't know to look for them (I often forget myself on other websites), and they shouldn't be relied as the sole way of conveying key info. Could you convey the info some other way as well? Ideas:

  • Next to/below the Topic Manual label, have some text saying the text must be edited in the .dockstore.yml.
  • When the user clicks the Edit button, a dialog pops saying edit the .dockstore.yml. I'm not 100% wild about this, but this is a brainstorm.

These are suggestions, not blocking, and it could stay the way it is as I suspect most users won't run into this.

@kathy-t kathy-t requested a review from svonworl October 31, 2023 15:39
Copy link

sonarcloud bot commented Oct 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kathy-t kathy-t merged commit ba1507d into develop Oct 31, 2023
20 checks passed
@kathy-t kathy-t deleted the feature/5625/github-app-topic branch October 31, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants