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

Reviewer Tools Pastebin Re-implementation #22640

Merged
merged 27 commits into from
Sep 13, 2024

Conversation

chrstinalin
Copy link
Contributor

@chrstinalin chrstinalin commented Sep 4, 2024

Fixes: mozilla/addons#14997

Description

Adds an AttachmentLog model and ability to either upload a file (currently limited to .txt) or directly paste a build log (stored as a .txt file) from any reviewer action. Attachments can then be downloaded in the add-on history.

image
image

Context

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.

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).

@chrstinalin chrstinalin marked this pull request as draft September 4, 2024 19:38
@chrstinalin chrstinalin changed the title Pastebin feature Reviewer Tools Pastebin Re-implementation Sep 4, 2024
@chrstinalin chrstinalin force-pushed the pastebin-feature branch 2 times, most recently from eef5a68 to 2c1b8f1 Compare September 9, 2024 17:32
@chrstinalin chrstinalin marked this pull request as ready for review September 10, 2024 19:38
@chrstinalin chrstinalin requested review from a team and eviljeff and removed request for a team September 10, 2024 19:39
src/olympia/reviewers/templates/reviewers/review.html Outdated Show resolved Hide resolved
src/olympia/reviewers/forms.py Outdated Show resolved Hide resolved
src/olympia/reviewers/forms.py Outdated Show resolved Hide resolved
src/olympia/reviewers/tests/test_utils.py Show resolved Hide resolved
src/olympia/reviewers/tests/test_utils.py Show resolved Hide resolved
src/olympia/reviewers/tests/test_views.py Show resolved Hide resolved
src/olympia/reviewers/utils.py Show resolved Hide resolved
@@ -43,6 +44,8 @@
re_path(r'^uploads/', include(upload_patterns)),
# Downloads.
re_path(r'^downloads/', include(download_patterns)),
# Activity.
re_path(r'^activity/', include(attachment_patterns)),
Copy link
Member

Choose a reason for hiding this comment

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

I'd lean towards putting the url under developers/ but don't know if @diox has a counter opinion.

Copy link
Member

Choose a reason for hiding this comment

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

activity/ makes slightly more sense to me, it's an activity attachment after all, and it's available to reviewers, not just developers.

The cleaner way would then be to:

  • Move the existing activity/urls.py to activity/api_urls.py
  • Re-create the activity/urls.py file and add a fresh urlpatterns definition to it with that new URL
  • Include it with re_path(r'activity/', include('olympia.activity.urls')) here.

Note that you would also need to add activity to SUPPORTED_NONAPPS and SUPPORTED_NONLOCALES settings (as well as the equivalent addons-frontend config variables)

Copy link
Member

Choose a reason for hiding this comment

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

(I was mainly thinking of a way of avoiding having to add another top level app/folder to configurations everywhere)

Copy link
Member

Choose a reason for hiding this comment

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

It's a little bit annoying but I think that's worth doing to avoid for consistency (and the less reviewers functionality depend on developer URLs/views/etc the better IMHO)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that you would also need to add activity to SUPPORTED_NONAPPS and SUPPORTED_NONLOCALES settings (as well as the equivalent addons-frontend config variables)

I did notice that doing this caused the download link to no longer work, but I'm assuming its due to the changes not being synced?

src/olympia/activity/models.py Outdated Show resolved Hide resolved
src/olympia/activity/views.py Show resolved Hide resolved
@eviljeff
Copy link
Member

Also, looking through the other tickets in the epic I don't see anywhere that explicitly mentions adding the link to the emails we send out. This patch doesn't add anything, and the developer hub ticket doesn't have it as a deliverable either.

@chrstinalin
Copy link
Contributor Author

Also, looking through the other tickets in the epic I don't see anywhere that explicitly mentions adding the link to the emails we send out. This patch doesn't add anything, and the developer hub ticket doesn't have it as a deliverable either.

Whoops, that's on me. It slipped my mind when I was making the tickets. I've added a new ticket mozilla/addons#15018 for the email.

src/olympia/reviewers/utils.py Outdated Show resolved Hide resolved
src/olympia/reviewers/tests/test_views.py Outdated Show resolved Hide resolved
src/olympia/reviewers/forms.py Show resolved Hide resolved
src/olympia/activity/urls.py Outdated Show resolved Hide resolved
chrstinalin added a commit to chrstinalin/addons-server that referenced this pull request Sep 13, 2024
Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

r+wc - just two minor things to change.

src/olympia/reviewers/forms.py Outdated Show resolved Hide resolved
src/olympia/reviewers/utils.py Outdated Show resolved Hide resolved
@chrstinalin chrstinalin merged commit c6026f6 into mozilla:master Sep 13, 2024
31 checks passed
chrstinalin added a commit that referenced this pull request Sep 17, 2024
* Added view attachment link on DevHub in relation to  #22640

* lint

* renamed attachment_link to attachment_url

* js attachment_url

* test update

* doc

* test

* dont hardcode test

* lint

* limit queries

* Use correct pk in reverse
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]: Reviewer Tools Pastebin Re-implementation
3 participants