Skip to content
This repository was archived by the owner on Oct 29, 2020. It is now read-only.

Forces a default run nid to be inserted into the filter#6194

Merged
itsjoekent merged 4 commits intoDoSomethingArchive:devfrom
itsjoekent-archive:6192
Feb 19, 2016
Merged

Forces a default run nid to be inserted into the filter#6194
itsjoekent merged 4 commits intoDoSomethingArchive:devfrom
itsjoekent-archive:6192

Conversation

@itsjoekent
Copy link
Contributor

What's this PR do?

Makes sure that the run nid on reportback view tab has a default value corresponding to the campaigns current default.

How should this be manually tested?

Go to a reportback review tab, is the correct thing filled out and does it show the correct reportbacks?

Any background context you want to provide?

I realized i didn't notice this the first time because on my machine the first ~10 reportbacks we're correct, but after that they were not. Whereas on Thor/Prod shit is just completely wrong and out of order.

What are the relevant tickets?

Fixes #6192

if ($form['#id'] == 'views-exposed-form-reportback-files-page-1') {
if (empty($_GET['run_nid'])) {
$run = dosomething_helpers_get_current_campaign_run_for_user(arg(1));
if (isset($run)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe !empty() will be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I only want this to run when the filter is empty. If I did !empty( it would run even when a user specifies a filter

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is setting the value for this filter

screen shot 2016-02-19 at 3 26 59 pm

However that should only happen when the filter is empty. If a value already exists in the filter, I don't want to replace it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I'm talking about asserting $run variable's existence.

Copy link
Contributor

Choose a reason for hiding this comment

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

if (!empty($run)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OHHHHHHHHHH

the if right above is empty and i thought thats what you were referring too.

The function im getting the run from returns NULL if it cant find it, so I dont think empty would work

Copy link
Contributor

Choose a reason for hiding this comment

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

But !empty(NULL) is FALSE, as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

And to be clear, dosomething_helpers_get_current_campaign_run_for_user() may return not just null, but FALSE and stdClass, see return node_load($run['id']);.

Copy link
Contributor

Choose a reason for hiding this comment

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

and FYI

php > $false = FALSE;
php > var_dump(isset($false));
bool(true)

@sergiitk
Copy link
Contributor

Also, instead of altering a form, would it make sense to use one of view's hooks, like hook_views_pre_build or something?

@itsjoekent
Copy link
Contributor Author

I've never gotten those view build hooks to work right, I find the form_alter to be much better

@sergiitk
Copy link
Contributor

Ok!

@sergiitk
Copy link
Contributor

My only concern is page-1 form id postfix. Would it be a case when this changes to page-2?

@itsjoekent
Copy link
Contributor Author

That was my first thought too when I noticed that, but no its always page-1. I have a feeling thats an old typo someone made haha

@sergiitk
Copy link
Contributor

26 comments - this escalated quickly.

@itsjoekent
Copy link
Contributor Author

@sergii-tkachenko ok, i think we're good now 😪

}


function dosomething_reportback_form_views_exposed_form_alter(&$form, &$form_state, $form_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot docblock 😉

@sergiitk
Copy link
Contributor

Who would think this PR turned out to be so large. High five, @deadlybutter!
👍

@itsjoekent
Copy link
Contributor Author

hall of fame much

itsjoekent pushed a commit that referenced this pull request Feb 19, 2016
Forces a default run nid to be inserted into the filter
@itsjoekent itsjoekent merged commit 5a6bc35 into DoSomethingArchive:dev Feb 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants