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

remove auto_add=now from last-modified field #7758

Merged
merged 3 commits into from
Nov 7, 2020

Conversation

hhio618
Copy link
Contributor

@hhio618 hhio618 commented Oct 25, 2020

Description

Fix bad test: app/marketing/tests/management/commands/test_ingest_community_events_calender.py

Refers/Fixes

Refs #7536, #7727

@codecov
Copy link

codecov bot commented Oct 25, 2020

Codecov Report

Merging #7758 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7758      +/-   ##
==========================================
+ Coverage   25.36%   25.46%   +0.09%     
==========================================
  Files         328      328              
  Lines       33082    33082              
  Branches     4926     4926              
==========================================
+ Hits         8391     8424      +33     
+ Misses      24408    24374      -34     
- Partials      283      284       +1     
Impacted Files Coverage Δ
app/marketing/models.py 66.78% <100.00%> (ø)
app/dashboard/embed.py 31.60% <0.00%> (+3.44%) ⬆️
...ement/commands/ingest_community_events_calender.py 69.49% <0.00%> (+45.76%) ⬆️

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 a3b1ab7...af9f064. Read the comment docs.

@@ -420,7 +420,7 @@ class UpcomingDate(SuperModel):
"""Define the upcoming date model"""
# These fields are meant to use for update UpcomingDate based on the icalendar updates
uid = models.CharField(max_length=255, null=True, blank=True)
last_modified = models.DateTimeField(db_index=True, auto_now=True)
last_modified = models.DateTimeField(db_index=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason for this? to the best of my knowledge, removing this means this field would no longer be updated when a record is changed.

If you're doing this because of failing tests, what you want to do is a unittest.mock

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 field needs to be filled manually from an input iCalendar file. I had added a auto_now=True to it accidentally. So yes definitely this attribute need to be deleted in my opinion.

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 field is different from modified_on that is defined in SuperModel and tracks last modification time of the object.

Copy link
Contributor

Choose a reason for hiding this comment

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

since we already have modified_on then we dont need those one as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As may you already knew, this field is related to the last_modified field of an actual event modification date in an iCal file @danlipert. I used this field to check if some event needs to get updated.

If I remove this field and use modified_on then there will be no modification date of actual db record, Is that ok?

@hhio618 hhio618 requested a review from amustapha October 28, 2020 07:30
@thelostone-mc thelostone-mc merged commit 8cbfa75 into gitcoinco:master Nov 7, 2020
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.

4 participants