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.

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

Adds possibility to define a goal description #9469

Closed
wants to merge 3 commits into from
Closed

Adds possibility to define a goal description #9469

wants to merge 3 commits into from

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jan 4, 2016

image

image

image

image

fixes #7346

@sgiehl sgiehl added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jan 4, 2016
tsteur
tsteur reviewed Jan 10, 2016
View reviewed changes
plugins/Goals/API.php
* @return void
*/
public function updateGoal($idSite, $idGoal, $name, $matchAttribute, $pattern, $patternType, $caseSensitive = false, $revenue = false, $allowMultipleConversionsPerVisit = false)
public function updateGoal($idSite, $idGoal, $name, $matchAttribute, $pattern, $patternType, $caseSensitive = false, $revenue = false, $allowMultipleConversionsPerVisit = false, $description = '')
{
Piwik::checkUserHasAdminAccess($idSite);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should keep the currently saved $description if $description = false and clear it if $description = ''. false could be default but I'm not sure what is better and whether is needed / more intuitive. I guess as caseSensitive and other optional parameters works the same as implemented it's all good and consistent the way it is implemented now 👍

@tsteur
Copy link
Member

tsteur commented Jan 10, 2016

A ui test fails here: http://builds-artifacts.piwik.org/piwik/piwik/goal_description/17418/UIIntegrationTest_goals_manage can you maybe have a look?

@sgiehl
Copy link
Member Author

sgiehl commented Jan 10, 2016

Guess the OmniFixture needs an update, so that the database contains the column. But I would wait until #9272 is merged.

@tsteur
Copy link
Member

tsteur commented Jan 10, 2016

OK :)

@tsteur
Copy link
Member

tsteur commented Jan 11, 2016

FYI: We're waiting for #9272

@mattab mattab modified the milestones: 3.0.0, 2.16.0 Jan 12, 2016
mattab
mattab reviewed Jan 12, 2016
View reviewed changes
plugins/Goals/templates/getOverviewView.twig
@@ -9,7 +9,7 @@
{% set conversion_rate=goal.conversion_rate %}
{% set name=goal.name %}
<div class="goalEntry" style="clear:both">
<h2>
<h2 {% if goal.description %}title="{{ goal.description }}"{% endif %}>
Copy link
Member

Choose a reason for hiding this comment

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

escaping is missing?

@mattab mattab modified the milestones: 3.0.0, 2.16.0, 3.0.0-b2 Jan 12, 2016
@mattab
Copy link
Member

mattab commented Jan 12, 2016

Targeting for 3.0 instead as it may be a little risky for 2.16.0?

@sgiehl sgiehl mentioned this pull request Apr 14, 2016
Merged
@sgiehl
Copy link
Member Author

sgiehl commented Apr 14, 2016

Replaced by #10057 which targets 3.x branch

@sgiehl sgiehl closed this Apr 14, 2016
@sgiehl sgiehl added wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it. and removed Needs Review PRs that need a code review labels Apr 14, 2016
@sgiehl sgiehl deleted the goal_description branch April 14, 2016 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Projects
None yet
Milestone
3.0.0-b2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sgiehl @tsteur @mattab