Use BaseResponse for proposal history to add API support #6039

njohner · 2019-11-11T15:09:44Z

In this PR we switch from a proposal specific history (using IProposalResponses) to the recently introduced opengever.base.response.Response. This allows us to simplify the code in the backend and provides API support for free.
This change is necessary to allow displaying of proposal history in the new frontend.

Also note that:

Responses do not get validated, for which I opened an issue: Missing validation for Responses #6050
created Field in Response was of the wrong type. This should not necessitate a migration as we keep the value but simply change the field type to match the value.
I changed the default and missing_value of the text field, to match the value set by default during initialization of the Response.

For #5934

Checkliste

Zutreffendes soll angehakt stehengelassen werden.

Sind UpgradeSteps deferrable, oder können gewisse Schritte des Upgrades konditional ausgeführt werden?
Changelog-Eintrag vorhanden/nötig?
Aktualisierung Dokumentation vorhanden/nötig?

njohner · 2019-11-13T10:27:32Z

opengever/api/field_deserializers.py

+
+@implementer(IFieldDeserializer)
+@adapter(IField, IPersistent, IBrowserRequest)
+class AnnotationsDefaultFieldDeserializer(DefaultFieldDeserializer):


Ok Sorry just realised that naming needs changing

njohner · 2019-11-13T10:27:39Z

opengever/api/field_deserializers.py

+
+@implementer(IFieldDeserializer)
+@adapter(IDatetime, IPersistent, IBrowserRequest)
+class AnnotationsDatetimeFieldDeserializer(DatetimeFieldDeserializer):


This one too

deiferni

Thank you for the excellent small commits, it really helped reviewing ✨!

I have added some small comments/suggestions, please have a look. Some comments might be outdated due to changes in later commits though.

In general i am a bit unhappy about getting rid of the single classes per history entry as that IMO decreases encapsulation. Instead of one single class we now do have "type"-check logic that is somewhat distributed across different places, e.g. ProposalResponseDescription.mapping and ProposalResponse.needs_syncing instead of having it encapsulated in one class/object. Not sure if that is a general decision made with how the Response now works and if the discussion is out of scope here. Maybe we can discuss it quickly at an upcoming daily or with some ☕️though?

Otherwise i'd give it a 👍.

deiferni · 2019-11-13T12:35:52Z

opengever/meeting/proposalhistory.py

+    implements(IProposalResponse)
+
+    def __init__(self, response_type='commented', text='', **kwargs):
+        super(ProposalResponse, self).__init__(response_type)


response type is a keyword argument, please also use a keyword argument here. Otherwise if Response suddenly introduces a positional argument code here might not fail, but should.

deiferni · 2019-11-13T12:41:37Z

opengever/meeting/proposalhistory.py

+            data[json_compatible(name)] = value
+        return data
+
+    def deserialize(self, data):


if you make this a factory-classmethod that receives a data dict you can move constructing the instance into the class. think of

response = ProposalResponse.deserialize(data, needs_syncing=False)

instead of

response = ProposalResponse() response.deserialize(data) response._needs_syncing = False

deiferni · 2019-11-13T12:44:33Z

opengever/meeting/proposalhistory.py

+        super(ProposalResponse, self).__init__(response_type)
+        self.text = text
+        self.additional_data = PersistentDict()
+        self.additional_data.update(kwargs)


why hide it in an additional dict? can't you just setattr(self, thing) for everything in kwargs?
Aks as you implement __getattr__ all the attributes that are present on self will shadow names in kwargs so that will lead to the same result.

update: aha no, it would probably not work with the deserialization/serialization, correct?

Yes, only fields that are defined in the schema get serialized, and I don't want to have all the possible fields in the schema.

deiferni · 2019-11-13T13:47:32Z

opengever/meeting/handlers.py

+        admin_unit_id,
+        '@@receive-proposal-history',
+        path=path,
+        data=request_data,)


trailing comma in method calls are IMO discouraged. please remove.

deiferni · 2019-11-13T13:48:29Z

opengever/meeting/proposalhistory.py

@@ -28,6 +28,7 @@
 from zope.schema import getFields


+NEED_SYNCING = (u'revised', u'reopened', u'commented')


i'd prefer if this was a constant on ProposalHistory as it is coupled.

ProposalHistory gets deleted in this PR... But I moved NEED_SYNCING into the ProposalResponse class

deiferni · 2019-11-13T13:54:55Z

opengever/meeting/browser/receiveproposalhistory.py

@@ -4,6 +4,7 @@
 from Products.Five import BrowserView
 from opengever.base.response import IResponseContainer
 from opengever.meeting.proposalhistory import ProposalResponse
+from opengever.meeting.activity.activities import ProposalCommentedActivitiy


typo, there is one i too much: ProposalCommentedActivitiy

Hum, that's how the class is defined. Correcting the classname should actually work without problem as these are not persisted objects?

i think so, yes.

deiferni · 2019-11-13T14:09:05Z

...gever/core/upgrades/20191108143533_migrate_proposal_history_to_proposal_responses/upgrade.py

+    def migrate_records_to_responses(self, context):
+        fields = getFields(IProposalResponse)
+
+        history = IAnnotations(context).get(annotation_key, None)


shouldn't we also delete the annotation_key from the objects annotation at the end?

I thought to keep them as backup of the data...

ok fair enough 👍

Made an issue to delete the data at a later time #6066

deiferni · 2019-11-13T14:09:53Z

opengever/meeting/browser/submitdocuments.py

@@ -208,6 +207,7 @@ def __call__(self):
                document_title=self.context.title,
                submitted_version=history_data['submitted_version'],
            )
+            response._needs_syncing = False


could we avoid setting a protected attribute from "outside". IMO that violates encapsulation.

Ok I added needs_syncing as argument to the __init__ of the ProposalResponse

deiferni · 2019-11-13T14:11:37Z

opengever/core/upgrades/20170704112234_migrate_sql_proposal_history_to_plone/upgrade.py

-)
-
-
-class PropsosalHistoryEntryMigrator(object):


we're sure the upgrades have been run for all deployments, right? otherwise we'd loose data here.

The oldest deployment is nidau with version 2018.5.7. This upgrade is in 2017.5.0, so 👍

deiferni · 2019-11-13T14:14:43Z

...gever/core/upgrades/20191108143533_migrate_proposal_history_to_proposal_responses/upgrade.py

@@ -54,6 +54,8 @@ def migrate_records_to_responses(self, context):
            history_type = record.pop('history_type')
            userid = record.pop('userid')
            text = record.pop('text')
+            if text is None:
+                text = ''


in contrary to what the commit message says this is not a unicode string 😉.

😓 Thanks

Field deserializers are only available for archetype and dexterity contents. Our response object is simply a Persisten object stored in the annotations.

created was a Date field in the IResponse schema but we store the created datetime on the Response object.

…ages.

These upgrade steps use the proposal history and will not work anymore. As they have been applied already on all our installations we can simply delete them.

njohner · 2019-11-18T15:02:54Z

...gever/core/upgrades/20191108143533_migrate_proposal_history_to_proposal_responses/upgrade.py

+    def migrate_records_to_responses(self, context):
+        fields = getFields(IProposalResponse)
+
+        history = IAnnotations(context).get(annotation_key, None)


Made an issue to delete the data at a later time #6066

njohner · 2019-11-18T15:05:06Z

opengever/core/upgrades/20171114105533_get_submission_date_from_proposal_history/upgrade.py

@@ -1,94 +0,0 @@
-from opengever.core.upgrade import SQLUpgradeStep


This upgrade is in 2017.7.0 so it's also ok to delete

njohner force-pushed the nj_proposal_history branch 3 times, most recently from b03d954 to da8557c Compare November 12, 2019 16:31

njohner mentioned this pull request Nov 13, 2019

Missing validation for Responses #6050

Open

njohner force-pushed the nj_proposal_history branch 2 times, most recently from c3452f9 to 0298ceb Compare November 13, 2019 09:36

njohner marked this pull request as ready for review November 13, 2019 10:27

njohner commented Nov 13, 2019

View reviewed changes

njohner force-pushed the nj_proposal_history branch 2 times, most recently from b779ae5 to bd180d6 Compare November 13, 2019 10:43

njohner requested a review from a team November 13, 2019 14:07

deiferni suggested changes Nov 13, 2019

View reviewed changes

Niklaus Johner added 17 commits November 18, 2019 14:59

Remove unused import.

02c2022

No need for same history record uid for submitted document.

2e1a69a

No need for same history record uid when creating submitted proposal.

1b367d3

No need for same history record uid when submitting document.

31c072c

Remove unused uuid parameter for Proposal.comment method.

0817caa

Add ProposalResponse class to replace BaseHistoryRecord.

5f097a7

Use ProposalResponse instead of HistoryRecord.

05cea34

Add field deserializer for Persistent objects.

458a29b

Field deserializers are only available for archetype and dexterity contents. Our response object is simply a Persisten object stored in the annotations.

Add serializer for ProposalResponse objects.

15fc4ec

Fix created field type in Response objects.

72ce277

created was a Date field in the IResponse schema but we store the created datetime on the Response object.

Add methods to serialize and deserialize a ProposalResponse.

600455c

Sync proposal responses when necessary.

ef09126

Add ProposalResponseDescription to help display correct response mess…

5d9628d

…ages.

Move ProposalResponse messages into ProposalResponseDescription.

0e8665f

Display ProposalResponses in overview instead of ProposalHistory.

e4a999b

Generate activity on proposal when commenting a submitted proposal.

66479fa

Clean up unused code in proposalhistory.

5dea02e

Niklaus Johner added 13 commits November 18, 2019 14:59

Add upgrade step to migrate proposal history to proposal responses.

40a1797

Sync proposal responses when submitting a document.

f91bc23

No need for ticking creation of proposal history in fixture.

d96e25b

Remove broken upgrade steps.

9880264

These upgrade steps use the proposal history and will not work anymore. As they have been applied already on all our installations we can simply delete them.

Limit created precision to seconds in ProposalResponse.

a0e2fc3

Adapt proposal history tests to test proposal responses instead.

ac14536

Add CL entry.

6d68b39

Add test for proposal GET over REST-API.

8d2f3d6

Add documentation proposals API endpoint.

dc7dd29

Also test responses in proposal creation over API.

83651fd

Make sure to use unicode string as default for ProposalResponse text.

28cb733

Set correct default value for text field in Response.

759318b

Make sure that text is set to empty string in ProposalHistory.

6e6e450

njohner force-pushed the nj_proposal_history branch from bd180d6 to 6e6e450 Compare November 18, 2019 14:03

njohner mentioned this pull request Nov 18, 2019

remove old proposal history data #6066

Closed

njohner commented Nov 18, 2019

View reviewed changes

Niklaus Johner added 2 commits November 18, 2019 16:09

Include corrections and comments from PR review.

8946475

Correct typo in ProposalCommentedActivity class name.

4634114

njohner requested a review from deiferni November 18, 2019 15:54

deiferni approved these changes Nov 18, 2019

View reviewed changes

njohner merged commit ce1cbbd into master Nov 19, 2019

njohner deleted the nj_proposal_history branch November 19, 2019 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use BaseResponse for proposal history to add API support #6039

Use BaseResponse for proposal history to add API support #6039

njohner commented Nov 11, 2019 •

edited

Loading

njohner Nov 13, 2019

njohner Nov 13, 2019

deiferni left a comment

deiferni Nov 13, 2019

deiferni Nov 13, 2019

deiferni Nov 13, 2019

njohner Nov 13, 2019

deiferni Nov 13, 2019

deiferni Nov 13, 2019

njohner Nov 18, 2019

deiferni Nov 13, 2019

njohner Nov 18, 2019

deiferni Nov 18, 2019

deiferni Nov 13, 2019

njohner Nov 18, 2019

deiferni Nov 18, 2019

njohner Nov 18, 2019

deiferni Nov 13, 2019

njohner Nov 18, 2019

deiferni Nov 13, 2019

njohner Nov 18, 2019

deiferni Nov 13, 2019

njohner Nov 18, 2019

njohner Nov 18, 2019

njohner Nov 18, 2019

		@@ -28,6 +28,7 @@
		from zope.schema import getFields


		NEED_SYNCING = (u'revised', u'reopened', u'commented')

		@@ -1,94 +0,0 @@
		from opengever.core.upgrade import SQLUpgradeStep

Use BaseResponse for proposal history to add API support #6039

Use BaseResponse for proposal history to add API support #6039

Conversation

njohner commented Nov 11, 2019 • edited Loading

Checkliste

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deiferni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njohner commented Nov 11, 2019 •

edited

Loading