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

Add check for job listing limit being 0 #2362

Merged
merged 6 commits into from
Jul 3, 2023
Merged

Conversation

mikeyarce
Copy link
Member

Fixes #2333

Changes proposed in this Pull Request

  • Add a check for job listing limit being 0. Currently if it's set to 0 it works the same as it being unlimited, which I don't think is intended.

Testing instructions

  • Create a few job listings
  • Set the limit to 0
  • Verify that you can't add more job listings

@mikeyarce mikeyarce requested review from gikaragia and a team and removed request for gikaragia March 6, 2023 19:41
Copy link
Contributor

@yscik yscik left a comment

Choose a reason for hiding this comment

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

Looks like this option is also used here where the same change should happen.

if (
$submit_job_form_page_id
&& $submission_limit
&& $job_count >= $submission_limit
) {

And is there a similar check on the actual job submission page and endpoint?

templates/job-dashboard.php Outdated Show resolved Hide resolved
templates/job-dashboard.php Outdated Show resolved Hide resolved
templates/job-dashboard.php Outdated Show resolved Hide resolved
@mikeyarce
Copy link
Member Author

@yscik - I tried the logic you are suggesting but it won't work if the Listing Limit is 0.

The logic I have now is:

  • Is this the job submit form page?
  • AND ( Are we below the limit OR is there a submission limit in place?)

Use Cases

No Limit

  • $submission_limit will be false
  • job_manager_count_user_job_listings() < $submission_limit will be false

0 Limit

  • $submission_limit will be false
  • job_manager_count_user_job_listings() < $submission_limit will be false

So in both cases we have both false, however they need to work in the opposite way.

I thought about adding this

if ( '0' == get_option( 'job_manager_submission_limit' ) ) {
	$submission_limit = true;
}

But, it seemed cleaner to just have a false === $submission_limit check. What do you think?

@gikaragia gikaragia requested review from yscik and a team June 2, 2023 10:04
@@ -23,7 +23,7 @@
exit; // Exit if accessed directly.
}

$submission_limit = get_option( 'job_manager_submission_limit' );
$submission_limit = ! empty( get_option( 'job_manager_submission_limit' ) ) ? absint( get_option( 'job_manager_submission_limit' ) ) : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

empty(0) and empty('0') is also true. I think we need '' !== get_option( 'job_manager_submission_limit', '' ) to test if it doesn't have any value set.

The if below checks for 'false' (string) instead of the false (bool) set here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in a8d3b34

@@ -87,7 +87,7 @@
<?php endforeach; ?>
<?php endif; ?>
</tbody>
<?php if ( $submit_job_form_page_id && ( job_manager_count_user_job_listings() < $submission_limit || ! $submission_limit ) ) : ?>
<?php if ( $submit_job_form_page_id && ( job_manager_count_user_job_listings() < $submission_limit || 'false' === $submission_limit ) ) : ?>
<tfoot>
Copy link
Contributor

Choose a reason for hiding this comment

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

There are other places in the plugin where this check needs to be updated. Might make sense to add a new job_manager_can_user_submit_job() to handle it consistently everywhere.

if (
$submit_job_form_page_id
&& $submission_limit
&& $job_count >= $submission_limit
) {

@mikeyarce
Copy link
Member Author

@yscik Thanks for the suggestions! I made a helper function job_manager_user_can_submit_job_listing to address this. I also added the check into the shortcode in e7b5f42

@yscik yscik added this to the 1.41.0 milestone Jun 27, 2023
Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

Suggested a small improvement. Other than that it looks good!

$submission_limit = get_option( 'job_manager_submission_limit', '' );
$job_count = job_manager_count_user_job_listings();

if ( '' !== $submission_limit &&
Copy link
Contributor

Choose a reason for hiding this comment

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

You can set the $can_submit variable only once with:

	$can_submit = '' === $submission_limit || $submission_limit > $job_count;

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @gkaragia ! Done in cc64267

Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

Looks good!

@mikeyarce mikeyarce merged commit 2a135ee into trunk Jul 3, 2023
7 checks passed
@mikeyarce mikeyarce deleted the fix/job-listing-limit-0 branch July 3, 2023 20:46
@gikaragia gikaragia changed the title Add check for job listing being 0 Add check for job listing limit being 0 Jul 4, 2023
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.

Listing Limit Option Not Working As Expected
3 participants