-
Notifications
You must be signed in to change notification settings - Fork 3
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
283 weekly roundup #360
283 weekly roundup #360
Conversation
Two sets of errors in automated tests, besides linting:
Those are (I think) the result of not having the following config values properly populated, though that surprises me given that I didn't notice those tests fail in #349 when I was looking at it earlier today:
The following look like either the pipeline's environment isn't able to stand up the SMTP sink server, and/or has a conflict on port 1025. Not sure how to proceed on that score, but if it's not a quick/obvious fix for someone else I'll dig into that later.
|
7b65024
to
6aaf897
Compare
@alexdunnjpl do we want to try to spin up a fake SMTP server, e.g. https://github.com/upgundecha/start-sendria-github-action? |
@alexdunnjpl if that doesn't work, I would just disable those test? or update the github action to ignore that test? |
|
||
def get_start_of_local_week() -> datetime: | ||
"""Return the start of the local-timezones week as a tz-aware datetime""" | ||
today = datetime.now().date() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to root ourselves to UTC here with datetime.datetime.now(datetime.timezone.utc)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. @jordanpadams ?
Current behaviour is to return data for the preceding Mon-Sun week, referenced to the tz of the host on which doi-service is running.
Your judgement call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nutjob4life @alexdunnjpl doesn't matter. weekly-ish is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jordanpadams I don't think that would work, as the test relies on capturing the stdout from the python smtp DebuggingServer, and you wouldn't want to introduce sendria as a dependency just for that test because it'd be a minor pain for local unit-testing. It would be possible to write a test that checked DebuggingServer output and sendria, and failed iff both fail, but that seems dirty and like a lot of effort just to support a few tests specific to one script which is unlikely to change often and which is/should-be subject to local unit testing anyway. My recommendation would be to set the tests as ignored for the time being and have me open a ticket to look into why the DebuggingServer isn't working, at a later date. |
@jordanpadams @nutjob4life can I get a sanity-check on this? yet
That env var should be set and available in the python environment, right? |
I can confirm that forked processes from the Roundup Action do properly inherit the environment. Roundup Action doesn't do anything special with the environment variables. I'm guessing that maybe But this test would fail even if self.assertEqual(os.environ.get('CI'), 'edunntag') Because Maybe just drop this test? Seems like a lot of fretting and consternation over something so small. EDIT: Yep, it's tox. Apparently there's a passenv option that tells what env vars to pass through from the running environment. |
Sorry, I was unclear - the edunntag thing was only there to force the test log to dump the value (in this case,
Ah, thanks - will set it up to pass through |
583d04a
to
38f34e2
Compare
Ready to merge as soon as Jordan has chimed in on the week timezone question. |
38f34e2
to
8b826e6
Compare
Ah OK. |
@alexdunnjpl so sorry to do this, but after talking to some other stakeholders, we came to the realization the modification date will not be accurate for "Available" date, since they may have modified it in September, but it was not released until December. Can we comment out that code for now until we have a better idea of how we will get this date from the metadata? |
User
Different PR/Issue, but sure can! |
🗒️ Summary
Adds a module/script for extracting recently-updated/submitted DOIs from the local database and sending an email notification to the user specified in
config['OTHER']['emailer_receivers']
. The email is comprised of an html summary, and a json attachment for the same records.⚙️ Test Data and/or Report
Unit tests pass. Comprehensive unit tests added for the new functionality.
♻️ Related Issues