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

[ENG-5206] v1 Profilewebsites added don't end up in NotableDomains #10524

Merged

Conversation

Johnetordoff
Copy link
Contributor

@Johnetordoff Johnetordoff commented Jan 30, 2024

Purpose

This fixes the issue where v1 social domains weren't being check correctly. We examined just adding another spam check, but decided to expand the checks to "check on save" behavior for users.

Changes

  • override user save to check dirty fields.
  • refactor tests to allow for new mocks.

QA Notes

Documentation

  • No external documentation updates are required.

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-5206

@Johnetordoff Johnetordoff marked this pull request as ready for review January 30, 2024 21:13
@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch 7 times, most recently from 8ea1cd4 to 2d58396 Compare February 8, 2024 21:41
@@ -76,7 +76,7 @@ def get_headers_from_request(req):
k: v
for k, v in headers.items()
}
headers['Remote-Addr'] = req.remote_addr
headers['Remote-Addr'] = getattr(req, 'remote_addr', None)
Copy link
Contributor Author

@Johnetordoff Johnetordoff Feb 8, 2024

Choose a reason for hiding this comment

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

Because the save and therefore spam check can now happen anywhere including outside the request context.

@@ -196,7 +196,7 @@ def do_check_spam(self, author, author_email, content, request_headers):
return

request_kwargs = {
'remote_addr': request_headers.get('Remote-Addr') or request_headers['Host'], # for local testing
'remote_addr': request_headers.get('Remote-Addr') or request_headers.get('Host'), # for local testing
Copy link
Contributor Author

@Johnetordoff Johnetordoff Feb 8, 2024

Choose a reason for hiding this comment

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

Because the save and therefore spam check can now happen anywhere including outside the request context.

@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch from 2d58396 to 250ef11 Compare February 8, 2024 21:44
@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch 9 times, most recently from ae858f9 to e577e5d Compare February 9, 2024 18:36
@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch 3 times, most recently from b019888 to 5db1c99 Compare February 9, 2024 21:00
…orOpenScience/osf.io into add-v1-social-spam-check

* 'add-v1-social-spam-check' of https://github.com/CenterForOpenScience/osf.io:
  move spam check into save method

# Conflicts:
#	osf/models/user.py
@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch 2 times, most recently from 7aead88 to 976abc8 Compare February 10, 2024 16:29
@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch from 976abc8 to fa2cd31 Compare February 10, 2024 16:39
Copy link
Member

@mfraezz mfraezz left a comment

Choose a reason for hiding this comment

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

One bug, a couple efficiency / code cleanliness items. Pass complete :octocat:

osf_tests/external/akismet/test_akismet.py Outdated Show resolved Hide resolved
tests/test_views.py Show resolved Hide resolved
osf/models/user.py Outdated Show resolved Hide resolved
osf_tests/test_user.py Outdated Show resolved Hide resolved
@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch 5 times, most recently from 5caa2a5 to 243f1ff Compare February 13, 2024 14:21
@Johnetordoff Johnetordoff force-pushed the add-v1-social-spam-check branch from 243f1ff to 742e34d Compare February 13, 2024 15:26
Copy link
Member

@mfraezz mfraezz left a comment

Choose a reason for hiding this comment

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

OsfTestCase/fixture incompatibilities aside, LGTM. I'll leave it to @jwalz to merge / determine inclusion in the next release, or if we want to do a smaller follow-up release afterwards. Pass complete :octocat:

@mfraezz mfraezz assigned mfraezz and jwalz and unassigned mfraezz Feb 14, 2024
@mfraezz mfraezz merged commit a250d1e into CenterForOpenScience:develop Feb 26, 2024
6 checks passed
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Mar 14, 2024
 into test

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (48 commits)
  [ENG-5282] metrics api docs (CenterForOpenScience#10545)
  Bump version and update CHANGELOG
  [ENG-5206] Parse v1 Profilewebsites for NotableDomains (CenterForOpenScience#10524)
  [EMG-4804] Cedar Project PR - BE (CenterForOpenScience#10498)
  [ENG-5136] datacite 4.5 (CenterForOpenScience#10529)
  ENG-5208 (CenterForOpenScience#10522)
  [ENG-5208]: Fix python bootstrapping in docker build (CenterForOpenScience#10518)
  [ENG-3696] Make gotoFileEvents always open in new tab (CenterForOpenScience#10482)
  [ENG-5011] Subject.get_semantic_iri  use the iri for a subject's bepress synonym only when it has the  same text -- it was instead doing the opposite
  [ENG-4335] subjects on projects (CenterForOpenScience#10324)
  fix: multiple funding awards from the same funder (CenterForOpenScience#10512)
  Add actions to update domain notes
  Avoid 401 when indexing withdrawn preprints
  use content instead of sanitized text for spam filter.
  Update sitemap for preprint routes, file downloads [ENG-4919]
  Add merge migration
  [ENG-4823] Add Collection Metadata Options (CenterForOpenScience#10499)
  Add merge migration
  Add UNVERIFIED Domain classification
  [ENG-4823] Add Collection Metadata Options (CenterForOpenScience#10499)
  ...
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