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

weather story uploads: overwrite upload #2091

Merged
merged 7 commits into from
Nov 27, 2024

Conversation

jamestranovich-noaa
Copy link
Collaborator

@jamestranovich-noaa jamestranovich-noaa commented Nov 18, 2024

What does this PR do? 🛠️

NB #2082 should be reviewed/merged first

Partially addresses #2033 -- specifically, by overwriting weather story images as they come in. I've also changed it so that uploads use the derived wfo for clarity.

What does the reviewer need to know? 🤔

I've added a logging message for this specific path. Example:

drupal-1             | [NOTICE] [weather_cms] [2024-11-18T22:06:32] weather story upload: moving public://National Weather Service/wfo_weather_story_upload/field_fullimage/fullimage1_43.png to public://National Weather Service/wfo_weather_story_upload/field_fullimage/fullimage1.png | uid: 2 | request-uri: http://localhost:8080/jsonapi/node/wfo_weather_story_upload
drupal-1             | [NOTICE] [weather_cms] [2024-11-18T22:06:32] weather story upload: moving public://National Weather Service/wfo_weather_story_upload/field_smallimage/smallimage1_36.png to public://National Weather Service/wfo_weather_story_upload/field_smallimage/smallimage1.png | uid: 2 | request-uri: http://localhost:8080/jsonapi/node/wfo_weather_story_upload

To test, please run outside tests twice and observe that /web/sites/default/files/unknown/wfo_weather_story_upload/field_fullimage/test-upload.png is overwritten properly.

@sspj-does-weather
Copy link
Collaborator

@eric-gade Is this the evidence you needed? (Asking because I'm not sure if these were the correct tests or not.)

(jt/overwrite-weather-stories)$ npx playwright test outside/*

Running 11 tests using 1 worker

  ✓  1 playwright/outside/api.spec.js:38:3 › API tests › users without credentials cannot get an API listing (480ms)
  ✓  2 playwright/outside/api.spec.js:56:3 › API tests › anonymous users cannot get an API listing (254ms)
  ✓  3 playwright/outside/api.spec.js:75:3 › API tests › uploader can see the API listing (535ms)
  ✓  4 playwright/outside/api.spec.js:86:3 › API tests › uploader can upload wfo pdfs (966ms)
  ✓  5 playwright/outside/api.spec.js:123:3 › API tests › uploader can upload wfo weather story images (2.1s)
  ✓  6 …tside/api.spec.js:167:3 › API tests › uploader cannot upload wfo weather story images with missing attributes (748ms)
  ✓  7 …right/outside/api.spec.js:268:3 › API tests › uploader can upload wfo weather story images with all attributes (1.3s)
  ✓  8 playwright/outside/api.spec.js:278:3 › API tests › drupal can derive wfo weather.gov url (1.4s)
  ✓  9 playwright/outside/api.spec.js:288:3 › API tests › drupal can derive wfo from relative WFO url (1.3s)
  ✓  10 playwright/outside/api.spec.js:298:3 › API tests › drupal can derive wfo from radar url (1.3s)
  ✓  11 playwright/outside/api.spec.js:308:3 › API tests › drupal can derive wfo from srh.noaa.gov url (1.3s)

  11 passed (12.4s)

@eric-gade
Copy link
Collaborator

@sspj-does-weather I'm going to add you as a reviewer. If you are able to do as the PR instructions say (that the image is saved, then ovewritten after running outside tests twice), then feel free to approve

Copy link
Collaborator

@sspj-does-weather sspj-does-weather left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Nice work, @jamestranovich-noaa! I left one optional suggestion.

drupal-test-1             | [NOTICE] [weather_cms] [2024-11-26T19:00:15] weather story upload: moving public://JAN/wfo_weather_story_upload/field_fullimage/test-upload_0.png to public://JAN/wfo_weather_story_upload/field_fullimage/test-upload.png | uid: 2 | request-uri: http://localhost:9080/jsonapi/node/wfo_weather_story_upload
drupal-test-1             | [NOTICE] [weather_cms] [2024-11-26T19:00:15] weather story upload: moving public://JAN/wfo_weather_story_upload/field_smallimage/test-upload_0.png to public://JAN/wfo_weather_story_upload/field_smallimage/test-upload.png | uid: 2 | request-uri: http://localhost:9080/jsonapi/node/wfo_weather_story_upload

web/modules/weather_cms/weather_cms.module Outdated Show resolved Hide resolved
@jamestranovich-noaa jamestranovich-noaa merged commit da5214b into main Nov 27, 2024
20 of 21 checks passed
@jamestranovich-noaa jamestranovich-noaa deleted the jt/overwrite-weather-stories branch November 27, 2024 17:38
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