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

4461 add file asset service visibility selector #4472

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Sep 10, 2024

Description

add file asset service visibility selector and integrate it with the existing api
added a new key on the staticAsset api to include the current folder
added the url column in the files list
added alerts

Related Issue(s)

branched out of #4473
closes #4461

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@cstns cstns self-assigned this Sep 10, 2024
@cstns cstns changed the base branch from main to 4449_add-a-directory-navigation-component September 10, 2024 08:35
@cstns
Copy link
Contributor Author

cstns commented Sep 10, 2024

Mobile is not yet optimized

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.14%. Comparing base (5dbdd3f) to head (0be3bd3).
Report is 33 commits behind head on 4449_add-a-directory-navigation-component.

Files with missing lines Patch % Lines
forge/ee/routes/staticAssets/index.js 90.00% 1 Missing ⚠️
Additional details and impacted files
@@                            Coverage Diff                             @@
##           4449_add-a-directory-navigation-component    #4472   +/-   ##
==========================================================================
  Coverage                                      78.13%   78.14%           
==========================================================================
  Files                                            296      296           
  Lines                                          14119    14129   +10     
  Branches                                        3189     3192    +3     
==========================================================================
+ Hits                                           11032    11041    +9     
- Misses                                          3087     3088    +1     
Flag Coverage Δ
backend 78.14% <90.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

…nto 4461_add-file-asset-service-visibility-selector
…bility-selector' into 4461_add-file-asset-service-visibility-selector
Comment on lines 54 to 55
// clear leading slash
return path.replace(/^\//, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is where the leading slash is being removed.

@hardillb
Copy link
Contributor

image

We have double slashes in the URLs

@hardillb
Copy link
Contributor

@cstns Sorry, I've just worked out why @joepavitt was using relative paths for files.

We can't use a standard /data/storage/ on LocalFS builds because the storage directory is going to be something like /opt/flowforge/var/projects/[project-id]/storage and we don't have an easy way to get that path in the UI.

So please revert my suggestion to have the full path for the Folder Path and the File Path both in what's displayed and when copied to the clip board. The paths should also not start with a leading / (which might fix the URLs having double / in the last comment).

Again sorry for making you change that earlier

@cstns
Copy link
Contributor Author

cstns commented Sep 11, 2024

Again sorry for making you change that earlier

No need to apologize, it's an easy revert.

Should I also revert the Base URL & File URL to relative paths or leave the fqdn as it is?

@hardillb
Copy link
Contributor

No, leave the urls please

Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

I'm saying this is good

@hardillb
Copy link
Contributor

But it has UI test failures so not merging

@cstns
Copy link
Contributor Author

cstns commented Sep 11, 2024

looking over them as we speak

@hardillb hardillb merged commit b7ada99 into 4449_add-a-directory-navigation-component Sep 11, 2024
14 checks passed
@hardillb hardillb deleted the 4461_add-file-asset-service-visibility-selector branch September 11, 2024 15:20
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.

Add a share/visibility selector and integrate it with the current API
3 participants