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

Show publishing api errors in publishing drawer #1367

Merged
merged 7 commits into from
Jun 9, 2022

Conversation

ChristopherChudzicki
Copy link
Contributor

Pre-Flight checklist

  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

#1366

What's this PR do?

This PR adds field-specific error messages to the publishing drawer, building on #1316

How should this be manually tested?

  1. Create a new site
  2. Try to publish the site using a url that is already in use
  3. You should see a specific error near the error field.
  4. If you want to check that generic / unexpected errors only show the "overall" error message... I am not sure the best way to trigger that. You could change the publishing URL in websitePublishAction code from "preview" to something invalid.

Screenshots (if appropriate)

Before / After
Screen Shot 2022-05-19 at 1 48 04 PM Screen Shot 2022-05-19 at 1 46 54 PM

throw new Error("Invalid PublishingEnv")
}

const PublishingOption: React.FC<PublishingOptionProps> = props => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was previously a function defined in PublishingDrawer, renderOption, now a real component.

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #1367 (fbf007d) into master (02af7ae) will increase coverage by 5.36%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
+ Coverage   89.10%   94.46%   +5.36%     
==========================================
  Files         232      111     -121     
  Lines        9733     3345    -6388     
  Branches     1843      795    -1048     
==========================================
- Hits         8673     3160    -5513     
+ Misses        932      177     -755     
+ Partials      128        8     -120     
Impacted Files Coverage Δ
static/js/components/PublishDrawer.tsx 95.38% <97.77%> (+0.20%) ⬆️
static/js/components/forms/PublishForm.tsx 93.54% <100.00%> (+0.21%) ⬆️
static/js/constants.ts 96.66% <100.00%> (-0.11%) ⬇️
static/js/query-configs/websites.ts 99.03% <100.00%> (+0.02%) ⬆️
static/js/testing_utils/IntegrationTestHelper.tsx 88.52% <100.00%> (+2.08%) ⬆️
websites/serializers.py
content_sync/apis/github.py
gdrive_sync/factories.py
gdrive_sync/constants.py
conftest.py
... and 117 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02af7ae...fbf007d. Read the comment docs.

@ChristopherChudzicki ChristopherChudzicki changed the title Cc/publishing errors 2 Show publishing api errors in publishing drawer May 19, 2022
@mbertrand mbertrand self-assigned this May 24, 2022
import { makeWebsiteDetail } from "../util/factories/websites"
import IntegrationTestHelper, {
import IntegrationTestHelperOld, {
Copy link
Member

Choose a reason for hiding this comment

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

Is there something that IntegrationTestHelperOld can test that the new one can't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. But the old one uses Enzyme, and the new one uses @testing-library/react ("RTL") (and other packges in the @testing-library family). RTL has a somewhat different API than Enzyme, so converting tests will be a bit of effort.

I've been trying to write new tests with RTL + the new helper, and have been converting old tests when refactoring significantly. (Which is natural, since the Enzyme tests tend to be a bit brittle, so refactoring tends to break them).

Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Works great, but looks like there is a conflict with master now so it needs a rebase.

replace two more constants with enums
specifically:
    - use spyOn to mock makeRequest, rather than a global mock. The global mock was being problematic when other imports (e.g., the old integration test helper) was also mocking `makeRequest`.
    - add `mockGetWebsiteDetail`
    - make `helper.mockRequest` more flexible in its argument matching
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

👍

@ChristopherChudzicki ChristopherChudzicki merged commit 04bd442 into master Jun 9, 2022
This was referenced Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants