Commit 149fdd1d authored by Jérome Perrin's avatar Jérome Perrin

credential: stop using deprecated property for email

In 44e0b22f (Move to new API, detailed properties take precedence,
2011-09-09) we introduce a new data model for coordinates, in the case
of emails, we have "url_string" fro the "detailed" form and
"coordinate_text", for the "store what user entered" form.

In some places of erp5_credential, we use setDefaultEmailText, which
calls Coordinate.setText which uses the deprecated Coordinate.fromText,
which sets the same value for "url_string" and "coordinate_text", which
seems a questionable behavior, because the data is saved twice and
some code might be using the wrong property.

This changes every usage to the new coordinate_text property
parent 5eeff828
Pipeline #26409 failed with stage
in 0 seconds
...@@ -30,7 +30,7 @@ person_mapping = ( ...@@ -30,7 +30,7 @@ person_mapping = (
('gender', 'gender'), ('gender', 'gender'),
('default_telephone_text', 'default_telephone_text'), ('default_telephone_text', 'default_telephone_text'),
('default_mobile_telephone_text', 'default_mobile_telephone_text'), ('default_mobile_telephone_text', 'default_mobile_telephone_text'),
('default_email_text', 'default_email_text'), ('default_email_coordinate_text', 'default_email_coordinate_text'),
('date_of_birth', 'start_date'), ('date_of_birth', 'start_date'),
('nationality', 'nationality'), ('nationality', 'nationality'),
('skill_list', 'default_career_skill_list'), ('skill_list', 'default_career_skill_list'),
......
...@@ -5,7 +5,7 @@ organisation = context.getDestinationDecisionValue(portal_type="Organisation") ...@@ -5,7 +5,7 @@ organisation = context.getDestinationDecisionValue(portal_type="Organisation")
#Mapping #Mapping
organisation_mapping = ( organisation_mapping = (
# (subscription, organisation) # (subscription, organisation)
('default_email_text', 'default_email_text'), ('default_email_coordinate_text', 'default_email_coordinate_text'),
('default_telephone_text', 'default_telephone_text'), ('default_telephone_text', 'default_telephone_text'),
('default_fax_text', 'default_fax_text'), ('default_fax_text', 'default_fax_text'),
('default_address_street_address', 'default_address_street_address'), ('default_address_street_address', 'default_address_street_address'),
......
...@@ -13,7 +13,7 @@ person_mapping = ( ...@@ -13,7 +13,7 @@ person_mapping = (
('date_of_birth', 'birthday'), ('date_of_birth', 'birthday'),
('nationality', 'nationality'), ('nationality', 'nationality'),
('language', 'language'), ('language', 'language'),
('default_email_text', 'default_email_text'), ('default_email_coordinate_text', 'default_email_coordinate_text'),
('default_telephone_telephone_country', 'default_telephone_telephone_country'), ('default_telephone_telephone_country', 'default_telephone_telephone_country'),
('default_telephone_text', 'default_telephone_text'), ('default_telephone_text', 'default_telephone_text'),
('default_fax_text', 'default_fax_text'), ('default_fax_text', 'default_fax_text'),
......
...@@ -33,7 +33,7 @@ if default_email_text is not None: ...@@ -33,7 +33,7 @@ if default_email_text is not None:
message = "We have sent you an email containing your username(s). Please check your inbox and your junk/spam mail for this email." message = "We have sent you an email containing your username(s). Please check your inbox and your junk/spam mail for this email."
if web_site: if web_site:
document_reference = web_site.getCredentialUsernameRecoveryMessageReference() document_reference = web_site.getCredentialUsernameRecoveryMessageReference()
createCredentialRecovery(default_email_text=default_email_text, createCredentialRecovery(default_email_coordinate_text=default_email_text,
destination_decision_value_list=person_list, destination_decision_value_list=person_list,
document_reference=document_reference, document_reference=document_reference,
language=portal.Localizer.get_selected_language()) language=portal.Localizer.get_selected_language())
......
...@@ -23,7 +23,7 @@ credential_request = module.newContent( ...@@ -23,7 +23,7 @@ credential_request = module.newContent(
default_credential_question_question=default_credential_question_question, default_credential_question_question=default_credential_question_question,
default_credential_question_question_free_text=default_credential_question_question_free_text, default_credential_question_question_free_text=default_credential_question_question_free_text,
default_credential_question_answer=default_credential_question_answer, default_credential_question_answer=default_credential_question_answer,
default_email_text=default_email_text, default_email_coordinate_text=default_email_text,
default_telephone_text=default_telephone_text, default_telephone_text=default_telephone_text,
default_mobile_telephone_text=default_mobile_telephone_text, default_mobile_telephone_text=default_mobile_telephone_text,
default_fax_text=default_fax_text, default_fax_text=default_fax_text,
......
...@@ -403,6 +403,8 @@ class TestERP5Credential(ERP5TypeTestCase): ...@@ -403,6 +403,8 @@ class TestERP5Credential(ERP5TypeTestCase):
person = person_result[0].getObject() person = person_result[0].getObject()
self.assertEqual(person.getTitle(), 'Homer Simpson') self.assertEqual(person.getTitle(), 'Homer Simpson')
self.assertEqual(person.getDefaultEmailText(), 'homer.simpson@fox.com') self.assertEqual(person.getDefaultEmailText(), 'homer.simpson@fox.com')
# the obsolete email property is not used
self.assertFalse(person.hasDefaultEmailUrlString())
# check homie can log in the system # check homie can log in the system
self._assertUserExists('homie', 'secret') self._assertUserExists('homie', 'secret')
...@@ -429,7 +431,7 @@ class TestERP5Credential(ERP5TypeTestCase): ...@@ -429,7 +431,7 @@ class TestERP5Credential(ERP5TypeTestCase):
last_name='Simpsons', # add a 's' to the end of the last_name last_name='Simpsons', # add a 's' to the end of the last_name
reference='homie', reference='homie',
password='new_password', password='new_password',
default_email_text='homie.simpsons@fox.com', default_email_coordinate_text='homie.simpsons@fox.com',
destination_decision=homie.getRelativeUrl()) destination_decision=homie.getRelativeUrl())
credential_update.submit() credential_update.submit()
...@@ -456,6 +458,7 @@ class TestERP5Credential(ERP5TypeTestCase): ...@@ -456,6 +458,7 @@ class TestERP5Credential(ERP5TypeTestCase):
self.assertEqual(related_person.getLastName(), 'Simpsons') self.assertEqual(related_person.getLastName(), 'Simpsons')
self.assertEqual(related_person.getDefaultEmailText(), self.assertEqual(related_person.getDefaultEmailText(),
'homie.simpsons@fox.com') 'homie.simpsons@fox.com')
self.assertFalse(related_person.hasDefaultEmailUrlString())
def stepCreateSubscriptionRequestWithSecurityQuestionCategory(self, sequence=None, def stepCreateSubscriptionRequestWithSecurityQuestionCategory(self, sequence=None,
sequence_list=None, **kw): sequence_list=None, **kw):
...@@ -544,7 +547,7 @@ class TestERP5Credential(ERP5TypeTestCase): ...@@ -544,7 +547,7 @@ class TestERP5Credential(ERP5TypeTestCase):
person = person_module.newContent(title='Barney', person = person_module.newContent(title='Barney',
reference='barney', reference='barney',
start_date=DateTime('1970/01/01'), start_date=DateTime('1970/01/01'),
default_email_text='barney@duff.com') default_email_coordinate_text='barney@duff.com')
# create an assignment # create an assignment
assignment = person.newContent(portal_type='Assignment', assignment = person.newContent(portal_type='Assignment',
function='member') function='member')
...@@ -862,6 +865,7 @@ class TestERP5Credential(ERP5TypeTestCase): ...@@ -862,6 +865,7 @@ class TestERP5Credential(ERP5TypeTestCase):
self.assertEqual("Homer", person.getFirstName()) self.assertEqual("Homer", person.getFirstName())
self.assertEqual("Simpson", person.getLastName()) self.assertEqual("Simpson", person.getLastName())
self.assertEqual("homer.simpson@fox.com", person.getDefaultEmailText()) self.assertEqual("homer.simpson@fox.com", person.getDefaultEmailText())
self.assertFalse(person.hasDefaultEmailUrlString())
self.assertEqual(DateTime('1970/01/01'), person.getStartDate()) self.assertEqual(DateTime('1970/01/01'), person.getStartDate())
self.logout() self.logout()
...@@ -878,6 +882,7 @@ class TestERP5Credential(ERP5TypeTestCase): ...@@ -878,6 +882,7 @@ class TestERP5Credential(ERP5TypeTestCase):
self.assertEqual("tom", person.getFirstName()) self.assertEqual("tom", person.getFirstName())
self.assertEqual("Simpson", person.getLastName()) self.assertEqual("Simpson", person.getLastName())
self.assertEqual("tom@host.com", person.getDefaultEmailText()) self.assertEqual("tom@host.com", person.getDefaultEmailText())
self.assertFalse(person.hasDefaultEmailUrlString())
self.assertEqual(DateTime('1970/01/01'), person.getStartDate()) self.assertEqual(DateTime('1970/01/01'), person.getStartDate())
def stepCheckPersonWhenCredentialUpdateFail(self, sequence=None, def stepCheckPersonWhenCredentialUpdateFail(self, sequence=None,
...@@ -1138,6 +1143,7 @@ class TestERP5Credential(ERP5TypeTestCase): ...@@ -1138,6 +1143,7 @@ class TestERP5Credential(ERP5TypeTestCase):
self.assertEqual(credential_request.getFirstName(), "Barney") self.assertEqual(credential_request.getFirstName(), "Barney")
self.assertEqual(credential_request.getDefaultEmailText(), self.assertEqual(credential_request.getDefaultEmailText(),
"barney@duff.com") "barney@duff.com")
self.assertFalse(credential_request.hasDefaultEmailUrlString())
self.assertEqual(credential_request.getRole(), "internal") self.assertEqual(credential_request.getRole(), "internal")
self.assertEqual(credential_request.getFunction(), "member") self.assertEqual(credential_request.getFunction(), "member")
......
  • @jerome in many cases (and in existing documents), we set the email address with setDefaultEmailText() and this change introduces compatibility issues.

    ipdb> person = self.portal.person_module.newContent(portal_type='Person')
    ipdb> person.setDefaultEmailText('foo@example.com') # we set email address like this in existing documents
    ipdb> person.default_email.url_string
    'foo@example.com'
    ipdb> person.default_email.coordinate_text
    'foo@example.com'
    ipdb> person.setDefaultEmailCoordinateText('bar@example.com') # this is credential update behaviour with this commit
    ipdb> person.default_email.url_string # this property is not updated
    'foo@example.com'
    ipdb> person.default_email.coordinate_text
    'bar@example.com'
    ipdb> person.getDefaultEmailText() # we still use this getter in many places, that returns the old value
    'foo@example.com'
    ipdb> person.getDefaultEmailCoordinateText()
    'bar@example.com'

    Do you mean we should update all getter/setter for email address in each project ?

  • Oh sorry, I did not consider the case of persons with an existing email address set as url string, thanks. This seems a big problem, because using erp5_credential with such emails does not update the email.

    Is it also your understanding that we want to use coordinate_text for new emails and that url_string is only for old documents ? When editing a person with Person_view, coordinate_text (only) is set. On very very old emails, url_string only is set (because before 44e0b22f there was no coordinate_text). On emails created with erp5_credential before this change - and more generally every time setText was called, both properties are set ( setText calls fromText and fromText sets both ). When both are set, the UI and getText return url_string and editing with UI edits url_string and the two properties become out of sync, which is the reason why I made this change, in a project data warehouse, email's getCoordinateText was used. I think in ERP5 everything uses setCoordinateText except here, I ran a script to compare all url_string and coordinate_text and only one was inconsistent, because of erp5_credential.

    When making this change, I was thinking we should always use getText when getting, because getText returns url_string if set and coordinate_text otherwise (it uses asText) - in my case there was also a bug in the datawarehouse code, it should be using getText instead of getCoordinateText to support both data models. But I did not consider the case of setting the value: when url_string is set, we want to edit url_string and only when it's not set we want to edit coordinate_text.

    I think this fromText could be made more clever, it's deprecated and I was scared of breaking something by changing it. I am now thinking of changing to something like this - and changing erp5_credential to use this same logic (maybe not using this method directly because it is deprecated):

    
      security.declareProtected(Permissions.ModifyPortalContent, 'fromText')
      @deprecated
      def fromText(self, text):
        """
        Sets url_string a.k.a. scheme-specific-part of a URL
        """
        if self.isDetailed():
          self.setUrlString(text)
        else:
          self.setCoordinateText(text)

    or maybe even like this:

    
      security.declareProtected(Permissions.ModifyPortalContent, 'fromText')
      @deprecated
      def fromText(self, text):
        """
        Sets url_string a.k.a. scheme-specific-part of a URL
        """
        if self.isDetailed():
          self.setUrlString(text)
          # In the past, using fromText was setting the value in both
          # coordinate_text and url_string properties, we remove the
          # duplication.  
          self.setCoordinateText('')
        else:
          self.setCoordinateText(text)

    what do you think ?

  • mentioned in commit cd24fb39

    Toggle commit list
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment