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

DevHub Pastebin Re-implementation #22664

Merged
merged 13 commits into from
Sep 17, 2024
Merged

Conversation

chrstinalin
Copy link
Contributor

@chrstinalin chrstinalin commented Sep 13, 2024

Fixes: mozilla/addons#14998

Description

Surfaces attachments on the DevHub side, allowing developers to download reply attachments.
image

Testing

  1. Enable the enable-activity-log-attachments waffle switch.
  2. Log in with [email protected] (for ease of navigation).
  3. Submit a new add-on.
  4. Make a reviewer reply (or any other action) from the review action section and include an attachment.
  5. Navigate to the 'Manage Status & Versions'. Click 'Review History' under 'Status' of the replied-to version.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@chrstinalin chrstinalin changed the title Added view attachment link on DevHub in relation to #22640 DevHub Pastebin Re-implementation Sep 13, 2024
@chrstinalin chrstinalin marked this pull request as draft September 13, 2024 15:14
@chrstinalin chrstinalin marked this pull request as ready for review September 13, 2024 20:12
@chrstinalin chrstinalin requested review from a team and KevinMind and removed request for a team September 13, 2024 20:12
@KevinMind
Copy link
Contributor

Trying to verify I caught something.. how is the URL being defined? I see the get_attachment_url using revers() but also saw in the browser a relative URL.

@diox can you clarify I can't remember if we need special care for ensuring the site URL is included as the URL root.

image

Also clicking the URL in local dev resulted in infinite redirects and eventually a failure.

Screenshot 2024-09-16 at 17 37 29

@chrstinalin
Copy link
Contributor Author

Also clicking the URL in local dev resulted in infinite redirects and eventually a failure.
@KevinMind

I pointed it out in the other PR, but it seems to have something to do with the constants? Removing them makes the link work as expected, but including them causes the infinite redirect. Since there was extra work to be done with frontend (syncing the constants), I had assumed the lack of constant-syncing was the issue.

@diox
Copy link
Member

diox commented Sep 16, 2024

On local environments, where requests are proxied through addons-frontend, it should work fine if your addons-frontend container is up to date with the changes from mozilla/addons-frontend@c09adf0

The reason it works if you remove the constants is that then it makes a redirect to /{locale}/activity/..., but we want to avoid that.

On dev/stage/prod we'll need SRE to update nginx config in https://github.com/mozilla-it/webservices-infra/blob/main/amo/k8s/amo-proxy/configs/addons.conf.tpl to make /activity be served by addons-server.

@KevinMind
Copy link
Contributor

@diox curious why did we not opt for /downloads root? This is for downloading an attachment.. I saw the conversation in @chrstinalin linked ticket but maybe I missed something.

@KevinMind
Copy link
Contributor

I just discovered we are currently not defaulting to pulling latest image of dependencies like addons-frontend that should probably be the case. I totally missed that my frontend was 7 days stale.

@KevinMind
Copy link
Contributor

@chrstinalin okay good news, new error. But still an error. now I'm getting RelatedObjectDoesNotExist looking for the attachment log.

I did the following:

  • navigate to listed review page
  • rejected the version with a "copy from pastebin" text I just entered random words
  • nvagiate to status and versions page
  • click view history
  • I see the attachment link and click
  • I see this error page.

Any idea what could be happening?

image

@chrstinalin
Copy link
Contributor Author

@KevinMind Whoops. Reverse is using the wrong ID

@diox
Copy link
Member

diox commented Sep 17, 2024

/downloads is not a bad idea, although currently, /downloads is defined from olympia.versions.urls, and this is not really tied to a version at all... and we already had activity views/URLs...

(Also, atm downloads is considered app-specific - we'd want to fix mozilla/addons#1871 before adding more stuff under that path IMHO)

Copy link
Contributor

@KevinMind KevinMind left a comment

Choose a reason for hiding this comment

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

Verified working locally. LGTM

@chrstinalin chrstinalin merged commit c7b2dc1 into mozilla:master Sep 17, 2024
31 checks passed
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.

[Task]: DevHub Pastebin Re-implementation
3 participants