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

Fix several issues in view feed of com_tags component #4651

Closed
wants to merge 2 commits into from
Closed

Fix several issues in view feed of com_tags component #4651

wants to merge 2 commits into from

Conversation

joomdonation
Copy link
Contributor

PR summary:

This PR fixes issue #4623 reported by @javigomez and several coding errors which I found while reviewing code in view feed of com_tags component:

  1. The getListQuery method of TagsModelTags component doesn't return "author" and "author_email", so there are notice errors in the following lines of code in view feed:

https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/views/tags/view.feed.php#L56
https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/views/tags/view.feed.php#L74

  1. $item->displayDate in the line of code https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/views/tags/view.feed.php#L57 need to be replaced with $item->created_time (there is no field displayDate in the $item object)
  2. $item in the following lines of code need to be replaced by $feeditem

https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/views/tags/view.feed.php#L70
https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/views/tags/view.feed.php#L74

  1. $row->author_email needs to be replaced by $item->author_email (I think it is typing mistake)

https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/views/tags/view.feed.php#L74

Testing instructions:

For some reasons, these notice errors are not displayed when I tried to view the feed, so it will be difficult for end-users to test this pull request (these errors are logged in error logs of the web server). Maybe one of PLTs (or developers who understand Joomla code) can help reviewing this code and get it merged? @mbabker Could you help review this code? The errors are clearly mentioned above.

For end-users, the only think you can test is:

  • View rss feed for tags component. You will see that for each feed item, the author and pubDate tag are not available for each feed item.
  • Apply this pull request. Check it again and you will see that author and pubDate are available now for each feed item.

@javigomez
Copy link
Contributor

Thanks for the fix @joomdonation ^_^

@joomdonation
Copy link
Contributor Author

You're welcome @javigomez . Hopefully this PR can be reviewed and merged (It is quite difficult to provide testing instructions for these kind of PRs :( ).

@rajeshstarlite
Copy link

@test, I've tested this issue, please find my test cases as screen shots:screen shot 2014-10-17 at 05 25 30screen shot 2014-10-17 at 05 25 36screen shot 2014-10-17 at 05 25 54

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4651.

@joomdonation
Copy link
Contributor Author

Thanks @rajeshstarlite for testing this PR. Unfortunately, I deleted the original repo and now I could not update the PR with new code. Could you please help making another change and test it again:

  1. Open the file components/com_tags/views/tags/view.feed.php
  2. Find the code below:
$feeditem->link        = '/index.php?option=com_tags&view=tag&id=' . (int) $item->id;
  1. Change it to:
$feeditem->link        = JRoute::_('index.php?option=com_tags&view=tag&id=' . (int) $item->id);

After that, please help test it again. If everything goes well, I will open a new PR to fix this issue.

For some reason, I could not see the author displayed as well. Maybe it depends on the rss reader which we are using. If you view the source of the feed, you will see the is generated like below (the author tag is available but not displayed)

<item>
<title>Components</title>
<link>http://localhost/jcms/index.php?option=com_tags&amp;view=tag&amp;id=2&amp;Itemid=582</link>
<guid isPermaLink="true">http://localhost/jcms/index.php?option=com_tags&amp;view=tag&amp;id=2&amp;Itemid=582</guid>
<description><![CDATA[<p>Components</p>]]></description>
<author>tuanpn@joomdonation.com (Super User)</author>
<category>All Tags</category>
<pubDate>Mon, 13 Oct 2014 03:59:05 +0000</pubDate>

Sorry for could not reply you earlier. It was my office time, so I could not work on it until now (after dinner :) )

Thanks again

@@ -127,6 +127,11 @@ protected function getListQuery()
->from($db->quoteName('#__tags') . ' AS a')
->where($db->quoteName('a.access') . ' IN (' . $groups . ')');

// Join over the users for the author and author_email field.
$query->select("CASE WHEN a.created_by_alias > ' ' THEN a.created_by_alias ELSE u.name END AS author")
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use a.created_by_alias = '' instead of a.created_by_alias > ' '?

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 Hannes. I am going to re-do this PR on tomorrow as I deleted the forked repo which I did this PR.

Regarding your suggestion, I think we can do that or use a.created_by_alias != '', not sure what's the prefered way (I took that code from one of the core component If I remember correctly).

Copy link
Contributor

Choose a reason for hiding this comment

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

@joomdonation Can you make this change please?

@joomdonation
Copy link
Contributor Author

@Hackwar @wilsonge I created a new pull request #5552 to replace this one (because I deleted the original repo and don't know how I can edit code without it). Please help checking the new PR when you have some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants