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.

Sign up for GitHub

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

Jump to bottom

[REF] Simplify activity import validation #19373

Merged
merged 1 commit into from
Jan 18, 2021
Merged

[REF] Simplify activity import validation #19373

merged 1 commit into from
Jan 18, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 12, 2021 •
edited
Loading

Overview

[REF] Simplify activity import validation

Before

Validation of field mapping done in 2 places

After

Validation of field mapping done in just 1 place

Technical Details

There are 2 types of validation on importing

  1. the correct fields have been mapped
  2. the specific row has all the required fields

These generally live in different places

  • field mapping is validated on the MapField::formRule validation function
  • the specific row is validated in the (badly named) 'summary' function.

However, the second function is also doing some validation of the
mapping, checking that either activity_type_id or label is mapped
& ditto activity_date_time. This duplicates the MapField rule
and is hard to read, while making a larger cleanup difficult

Comments

@civibot
Copy link

civibot bot commented Jan 12, 2021

(Standard links)

@civibot civibot bot added the master label Jan 12, 2021
demeritcowboy
demeritcowboy reviewed Jan 13, 2021
View reviewed changes
CRM/Activity/Import/Parser/Activity.php
@@ -151,23 +151,14 @@ public function summary(&$values) {
$this->setActiveFieldValues($values, $erroneousField);
$index = -1;

if ($this->_activityTypeIndex > -1 && $this->_activityLabelIndex > -1) {
array_unshift($values, ts('Please select either Activity Type ID OR Activity Type Label.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I read this as NOT being a duplicate of what happens in mapfield formRule. And when I run I get two different things before and after the patch. What this line is saying is you can't select BOTH id and label for activity type in the same mapping. Whereas in mapfield it's checking that you have to pick at least one. The error message here is a little ambiguously worded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah OK - I suppose that is a valid error - I hadn't picked that up from the code - but we should probably only error if they conflict

@eileenmcnaughton eileenmcnaughton force-pushed the valid branch 2 times, most recently from c94a8ac to f6b5991 Compare January 15, 2021 07:58
eileenmcnaughton
eileenmcnaughton commented Jan 15, 2021
View reviewed changes
CRM/Activity/Import/Parser/Activity.php Outdated
throw new CRM_Core_Exception(ts('Activity label and Activity Type ID are in conflict'));
}
if (!$activityLabel && !$activityTypeID) {
throw new CRM_Core_Exception(ts('Missing required fields') . ' ' . implode(ts(' or '), [ts('Activity label'), ts('Activity Type id')]));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this just because I thought it might avoid adding a new string to be translated

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand what you're saying but most of these strings don't exist currently, and ' or ' might be awkward to translate without context.

Also for this one and the error on 458 it should be "activity type label" not "activity label".

Copy link
Member

Choose a reason for hiding this comment

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

Also, as a rule, leading and trailing whitespace is not allowed in strings passed to ts().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - I've pushed up as a single string

@eileenmcnaughton
Copy link
Contributor Author

I've updated this to still validate the type or label fields

@eileenmcnaughton eileenmcnaughton force-pushed the valid branch 3 times, most recently from 1332eb0 to d6c3af6 Compare January 17, 2021 06:16
@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor Author

test this please

demeritcowboy
demeritcowboy reviewed Jan 18, 2021
View reviewed changes
CRM/Activity/Import/Parser/Activity.php
* @return null|string
*/
protected function getFieldValue(array $row, string $fieldName) {
return $row[$this->getFieldIndex($fieldName)] ?? NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to check if $this->getFieldIndex($fieldName) returns false, because suppose I map type id but not type label, then the false gets cast to element 0, and then on line 454 it's wrong and the if just below that does the wrong thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy I've updated it

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
  • Developer standards
    • [r-tech] PASS
    • [r-code] Issue
      • The ts() wrangling doesn't really save anything and seems awkward. In fact what happens is half the string is translated since the other strings are new strings not already translated. And ' or ' on its own has no context for the translator.
      • It should be "activity type label" not "activity label" in both lines 461 and 464.
      • Non-blocking comment: Technically it's possible to have string '0' as an activity type label which breaks line 459, but if people are doing that then the right answer is they should stop doing that.
    • [r-maint] ____
    • [r-test] PASS

@eileenmcnaughton
[REF] Simplify activity import validation
c5a8afa 
There are 2 types of validation on importing
1) the correct fields have been mapped
2) the specific row has all the required fields

These generally live in different places
- field mapping is validated on the MapField::formRule validation function
- the specific row is validated in the (badly named) 'summary' function.

However, the second function is also doing some validation of the
mapping, checking that either activity_type_id or label is mapped
& ditto activity_date_time. This duplicates the MapField rule
and is hard to read, while making a larger cleanup difficult
@demeritcowboy
Copy link
Contributor

Looks good now!

@eileenmcnaughton
Copy link
Contributor Author

This is passing now & has thumbs up from @demeritcowboy - @colemanw @seamuslee001 could one of you merge?

@seamuslee001
Copy link
Contributor

Merging based on @demeritcowboy 's review

@seamuslee001 seamuslee001 merged commit 6ed916e into civicrm:master Jan 18, 2021
@seamuslee001 seamuslee001 deleted the valid branch January 18, 2021 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demeritcowboy demeritcowboy demeritcowboy left review comments

@colemanw colemanw colemanw left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eileenmcnaughton @demeritcowboy @seamuslee001 @colemanw