In commons we're manually attempting to create our own count of how many messages a user is subscribed to. This has proven to be unreliable. It'd be much easier if message subscribe returned the count along with the resultset when queried.

The attached patch does just that, and allows for API backwards compatibility.

CommentFileSizeAuthor
#6 2206157-message-subscribe-row-count-3.patch3.75 KBjaperry
#4 2206157-message-subscribe-row-count-2.patch2.76 KBjaperry
#1 2206157-message-subscribe-row-count.patch1.23 KBjaperry
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Status: Active » Needs review
FileSize
2206157-message-subscribe-row-count.patch1.23 KB
ezra-g’s picture

ezra-g CreditAttribution: ezra-g commented

The new parameter changes the format of the return value.

If the return value is currently a view object, why not just return a view with the result count included when the new parameter is used?

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented
Status: Needs review » Closed (won't fix)

Sorry, but I don't like this approach much as it hardcodes a certain scenario. You could always hook_views_pre_render() and "register" the count in another static variable.

Please re-open if you disagree or think I missed your point :)

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Status: Closed (won't fix) » Needs review
FileSize
2206157-message-subscribe-row-count-2.patch2.76 KB

Agreed, we shouldn't be hard-coding our use case.

However, it'd be nice to not have to copy out the code from message_subscribe just because we need another view function. I've made the message_subscribe_ui_tab function more of a wrapper, so other modules can call the view on its own if needed.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented
Title: Return a result count from message subscribe UI tab » Add an API function to grab the view object of a message subscribe page
Status: Needs review » Needs work

This makes sense.

  1. +++ b/message_subscribe_ui/message_subscribe_ui.module
@@ -136,6 +136,22 @@ function message_subscribe_ui_tab($account, $flag_name = NULL) {
+  return $view->preview();

    This should probably be
    return $view ? $view->preview() : '' 

  2. +++ b/message_subscribe_ui/message_subscribe_ui.module
@@ -136,6 +136,22 @@ function message_subscribe_ui_tab($account, $flag_name = NULL) {
+ * API Call to grab the view object of a message subscribe page.

    Maybe better: Return the View object ... 

  3. +++ b/message_subscribe_ui/message_subscribe_ui.module
@@ -136,6 +136,22 @@ function message_subscribe_ui_tab($account, $flag_name = NULL) {
+ *   The user object

    Missing dot here and below.

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Title: Add an API function to grab the view object of a message subscribe page » Add an API function to grab the view object of a message subscribe page, fix page count
Status: Needs work » Needs review
FileSize
2206157-message-subscribe-row-count-3.patch3.75 KB

So there are two issues here, but they're somewhat intertwined and its really annoying to split them out. The count issues we were finding with commons actually exists in message subscribe as well.

However, message subscribe is including the title in the page callback, whereas follow_ui is doing it in a quicktab. I don't believe we need to abstract out the title parameters, but the following patch will align the count to the view display.

Fixed 1) and 3) mentioned in #5 .. Were you thinking return the view object in message_subscribe_ui_tab? I don't think we should do that, as it would break backwards compatibility. message_subscribe_ui_tab should continue to return the view content, and if other modules want the object, they should call the new API function instead. (Eg: #2201909)

The following patch cleans up the count, abstracts the view object, and does some other cleanup to typos in pre-existing in the module.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Looks good, but one question:

+++ b/message_subscribe_ui/message_subscribe_ui.module
@@ -103,19 +103,22 @@ function message_subscribe_ui_tab_title($account, $flag_name = NULL) {
-  $flaggings = flag_get_user_flags($entity_type, NULL, $account->uid);

Can you clarify why this is wrong, and using the Views count is better -- is it because you customize the View?

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented

flag_get_user_flags does not have any access checks. Therefore, if you unpublish a node you're following, the count will include that node, even though the view does not.

We've had other oddities as well, including customizing the view. But in general, the count really should be derived from whatever is displaying on the page. Trying to rebuild the count another way is error prone.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented
Status: Needs review » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.