Skip to content

Feature default project for new report#327

Merged
kbeker merged 3 commits intomasterfrom
feature-default-project-for-new-report
Jun 26, 2019
Merged

Feature default project for new report#327
kbeker merged 3 commits intomasterfrom
feature-default-project-for-new-report

Conversation

@rwrzesien
Copy link
Contributor

Resolves #29

TODO: Add test that ReportForm.projects field sets initial to project in which last report was created after #296 is merged.

Copy link
Contributor

@dybi dybi left a comment

Choose a reason for hiding this comment

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

baba_jaga_mysli
Please consider/explain.

def get_project_ordered_by_last_report_creation_date(self) -> QuerySet:
return self.projects.annotate(
last_report_creation_date=Coalesce(
Max("report__creation_date", filter=Q(report__author=self)), Value("1970-01-01 00:00:00")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if I understand why we need Coalesce and Max here... Could you please explain?

Copy link
Contributor Author

@rwrzesien rwrzesien Jun 24, 2019

Choose a reason for hiding this comment

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

If given project has no reports created for current user, the returned value from Max() is None, which then is being put at the beginning of the list (because we use DESC ordering). So to avoid that I am using Coalesce to replace None with oldest date known to human civilization and get the desired order.



class TestGetProjectOrderedByLastReportCreationDate(TestCase):
def setUp(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

personally I would prefer having projects build with ProjectFactory, i.e

project1 = ProjectFactory()

and then have reports named like

self.project1_report1 = ReportFactory(author=self.user, project=self.project1)

this way the tests would be easier to grasp 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.

👍 done, more code, but as you have said, easier to read :)

@rwrzesien rwrzesien force-pushed the feature-default-project-for-new-report branch from edc0405 to fc11a46 Compare June 24, 2019 22:54
@dybi dybi self-requested a review June 25, 2019 12:05
Copy link
Contributor

@dybi dybi left a comment

Choose a reason for hiding this comment

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

yes

@rwrzesien rwrzesien force-pushed the feature-default-project-for-new-report branch from fc11a46 to 5b2a28d Compare June 25, 2019 14:49
@kbeker kbeker force-pushed the feature-default-project-for-new-report branch from 5b2a28d to 4269ecf Compare June 26, 2019 11:41
@kbeker kbeker merged commit 4269ecf into master Jun 26, 2019
@kbeker kbeker deleted the feature-default-project-for-new-report branch June 26, 2019 11:50
@kbeker kbeker added this to the next_release milestone Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default project for new report

3 participants