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

feat: remote pins on files page #1721

Conversation

rafaelramalho19
Copy link
Contributor

Closes #1502

@rafaelramalho19 rafaelramalho19 self-assigned this Feb 9, 2021
@rafaelramalho19 rafaelramalho19 changed the base branch from master to feature/remote-pinning-settings-page February 9, 2021 15:24
@lidel lidel mentioned this pull request Feb 10, 2021
@lidel lidel changed the title Feat/remote pins files page feat: remote pins on files page Feb 10, 2021
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Not sure if issues I had were caused by using different version of ipfs-http-client with connect-deps, but here is quick feedback:

  • error handling seems to be missing. when I unpinned a file from one of services that are broken on purpose, I got this page crash

  • assuming http-client issues caused below ones, but mentioning them for completeness:

    • I was not able to get cloud pin to show on Files screen
    • I have multiple services defined with Pinata endpoint, but I don't see them in "Set pinning" modal, I only see a single Pinata entry

@rafaelramalho19 we should be able to remove the need for connect-deps and use ipfs-http-client 49.x (it shipped with pin.remote), but may require fixes from #1655

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

We merged latest http-client with #1655 so connect-deps should no longer be necessary.

@rafaelramalho19 mind rebasing this PR?

@lidel lidel added the P1 High: Likely tackled by core team if no one steps up label Feb 25, 2021
@lidel
Copy link
Member

lidel commented Mar 4, 2021

I've merged fixes from main branch but there is some remaining work here:

@rafaelramalho19 rafaelramalho19 force-pushed the feat/remote-pins-files-page branch 2 times, most recently from 1944510 to 1cd2c2d Compare March 12, 2021 14:17
@rafaelramalho19
Copy link
Contributor Author

@lidel > when I have multiple Pinata services the "Set pinning" modal shows "Pinata" only once (we need to merge #1713 first and rebase)

This was actually never done apparently, ıt was so long ago I can't remember but I guess I didn't finish it, so I done it today 😄

I also replicated the colors from the settings page here
image

@rafaelramalho19
Copy link
Contributor Author

when one of services is offline entire Files screen crashes

  • What's the best way to reliably test this? Should I make an e2e test and try to reproduce this behavior there?
  • It crashes when in dev mode right? If you "close the debugging modal" it should continue as normal right?

rebase after #1535 is merged

What do you mean by this? That PR was merged a long time ago 🤔

@lidel
Copy link
Member

lidel commented Mar 12, 2021

when one of services is offline entire Files screen crashes

This seems to be fixed in the latest version: the screen no longer crash.
Instead, when I pin a file to a service which is offline, no error is displayed and the file gets cloud pin badge, buf if I refresh the page the badge disappears.

What's the best way to reliably test this? Should I make an e2e test and try to reproduce this behavior there?

For dev just add a service with invalid endpoint or access token (so every request always fail).

I think the only way to do automatic tests is to add test/e2e/remote-pins.test.js with tests for adding/removing service and pinning files using invalid service and a valid one.

For valid one you can use js-mock-ipfs-pinning-service:

  • runs in memory, so should be acceptable for CI
  • you can force it to set status to 'pinned' by using CID with identity multihash like bafkqaaa (you can import it from IPFS via Files→Import→Import from IPFS → /ipfs/bafkqaaa)

That PR was merged a long time ago

My bad, meant #1713 :)

Gozala and others added 8 commits March 15, 2021 13:28
This PR adds visual feedback for then big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>
@rafaelramalho19 rafaelramalho19 force-pushed the feat/remote-pins-files-page branch from 011decd to 4337f3b Compare March 15, 2021 13:29
@rafaelramalho19 rafaelramalho19 requested a review from lidel March 15, 2021 13:30
@rafaelramalho19 rafaelramalho19 merged commit 18620eb into feature/remote-pinning-settings-page Mar 15, 2021
@rafaelramalho19 rafaelramalho19 deleted the feat/remote-pins-files-page branch March 15, 2021 13:52
rafaelramalho19 added a commit that referenced this pull request Mar 19, 2021
* feat: upload progress ui (#1655)

This PR adds visual feedback for then big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: rollback connect-deps

* chore: go-ipfs 0.8.0-rc1

* feat: integrate remote pinning in the files page

* chore: update deps

* chore: linting fixes

* feat: add pinning services to the pinning modal

* chore: update conflicts

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit that referenced this pull request Mar 26, 2021
* chore: WIP in settings page
* feat: add remote pinning services to the settings page
* chore: memoise pinning service color
* chore: set the same colors for pinning services manager items
* chore: update src/components/pinning-manager/PinningManager.js
* chore: remove console.log
* chore: make pinning work in go-ipfs 0.7 & add docs link
* chore: move pinning constants to another file
* feat: detect if remote services are available in the settings page
* chore: switch to production Pinata
https://pinata.cloud/documentation#PinningServicesAPI

* feat: remote pins on files page (#1721)
* feat: upload progress ui (#1655)

This PR adds visual feedback for when big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: rollback connect-deps

* chore: go-ipfs 0.8.0-rc1

* feat: integrate remote pinning in the files page

* chore: update deps

* chore: linting fixes

* feat: add pinning services to the pinning modal

* chore: update conflicts

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: disable js-ipfs tests for the time being

* feat: add autoUpload toggle to the pinning manager

* chore: change autoupload labels

* refactor: simplify size calculations

Using pre-existing stat helper makes webui immune to unexpected
exceptions.

Pin size calculation was removed, because it was already hidden in GUI
of Settings screen.

* fix: pin.remote.ls status check

This fixes a racy bug: when a single service was offline,
any remaining services were not checked due to error thrown.

This usually did not happen in Chromium, but failed pretty often in
Firefox, which seem to execute looped for awaits bit differently.

* fix: meaningful labels and variable names

* fix: auto upload labels

- basic explainer whatpolicy change means
- enable/disable
- manual/all files

* fix: disable pinning to offline services

- filled missing labels with basic explainer
  (can be improved in separate PRs)
- disabled pinning to services that are offline + added visual
  indication when there is a problem with service

* fix: pinning service templates

- sets icon and docs url when name includes template name

* style: auto upload modal title

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

* style: auto upload modal description

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

* style: auto upload menu item

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

* fix: no online check for local pinning

* style: separate title for remote pin glyph

* chore: cleanup

Co-authored-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pinning integration: Make Files screen functional
3 participants