Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Ophase Categories #129

Merged
merged 23 commits into from
Feb 7, 2017
Merged

Ophase Categories #129

merged 23 commits into from
Feb 7, 2017

Conversation

bhaettasch
Copy link
Member

@bhaettasch bhaettasch commented Feb 1, 2017

Introducing and using different categories for Ophase to e.g. allow a Ophase with only Master tutor groups (already possible without these changes) but also only Jobs for a Master Ophase (using changes from this branch/pull request).

Work in progress. Feel free to comment anyway.

This aims to resolve #56

@bhaettasch bhaettasch added this to the ophase-ss17 milestone Feb 1, 2017
bhaettasch and others added 6 commits February 1, 2017 23:44
Accessible through ManyToMany relation with additional fields
Start and end date are calculated automatically
Fixed naming issue (renamed field from begin_date to start_date for higher consistence) in OphaseActiveCategory
Ophases have to be named now explicitly to resolve recursive call issues (of ophase and the intermediate table). Removed obsolete related methods
@bhaettasch
Copy link
Member Author

Not sure why the tests are failing with

ValueError: Related model 'ophasebase.OphaseCategory' cannot be resolved

suddenly. Has anyone an idea? Maybe @ckleemann?

Also I am a little bit annoyed by those fake "new issues" of codeclimate from lots of files I did not even touch.

bhaettasch and others added 5 commits February 2, 2017 23:54
For some migrations, a specific order had to be enforced manually
Added CASCADE deletion constants for foreign keys. Required in Python20
Added deletion constants for foreign keys. Required in Python20. Also changed null constraints.
This commit may be cherry picked as it is not related to the features of this branch but was created during removing all warnings.
@bhaettasch
Copy link
Member Author

I was able to track down the error -- it was caused by migrations depending on a specific run order that was used locally during incremental migrations but not when populating a new database. No need to check anymore, @ckleemann.

Copy link
Member Author

@bhaettasch bhaettasch left a comment

Choose a reason for hiding this comment

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

@bhaettasch
Copy link
Member Author

Contrary to the commit message of bd5c9e2, this commit cannot be cherry picked since a migration is contained (and the dependencies would not be fulfilled when picking).

@bhaettasch bhaettasch mentioned this pull request Feb 3, 2017
2 tasks
@schroedingersZombie
Copy link
Contributor

We would like to deploy this features asap so that the hompage of pyOphase can be updatet to the next Ophase.

Copy link
Member

@jlauinger jlauinger left a comment

Choose a reason for hiding this comment

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

  • Have you tested that there aren't any regressions with the PDF export, esp. with human readable dates?
  • You might wanna squash migrations for ophasebase and staff
  • Apart from that lgtm 👍

@@ -13,10 +13,10 @@ class OphaseIsActive(TestCase):

def test_only_one_is_active(self):
# create Ophase - should be active
o1 = Ophase.objects.create(start_date=date(2014, 4, 7), end_date=date(2014, 4, 11), is_active=True)
o1 = Ophase.objects.create(name="Testophase 1", is_active=True)
Copy link
Member

Choose a reason for hiding this comment

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

It appears to me as if this only tests the Ophase instance itself, not any OphaseCategories. You might want to add some OphaseActiveCategories to o1.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created issue #131 to make sure this does not fall into oblivion.

Copy link
Member

@exploide exploide left a comment

Choose a reason for hiding this comment

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

Just had a quick look.

  • +1 for squashing migrations
  • it seems the translations are incomplete

@bhaettasch
Copy link
Member Author

Reports are working, I just checked it.

Squashing is probably not a good idea since I had to edit the migrations manually to make sure migrating works for existing databases as well as empty ones.

The translations are already done on the main branch (with #128) and will be complete after merging.

@bhaettasch
Copy link
Member Author

From my point of view, the essential work is done here and this branch can be merged. All further improvements and usages are enhancements and can be added when needed.

@bhaettasch bhaettasch changed the title WIP: Ophase Categories Ophase Categories Feb 6, 2017
bhaettasch and others added 2 commits February 7, 2017 01:01
The property is currently used to display the block on the frontpage of the website using the main language of the OphaseCategory
@jlauinger jlauinger merged commit c4c7a41 into master Feb 7, 2017
@jlauinger jlauinger deleted the ophaseCategories branch February 7, 2017 21:05
@bhaettasch bhaettasch restored the ophaseCategories branch February 8, 2017 09:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link Jobs with Groupcategories
6 participants