Format plural for number of installs

Needs to be rebased against 2.0.x...

Rebased

One question, otherwise I don't see any obvious problem with this.

Required changes done.

Not quite done...

That looks correct to me - I think we'd want to use project.project_usage_total since formatPlural() is treating it as a number, not a string. RTBC assuming tests pass!

The code change looks good, but should this have test coverage? Pretty sure if this was in core a test would be required.

Updated fixture to make the total installs of the Grapefruit module 1. Required some minor adjustments to existing tests.

Added testInstallCountPluralFormatting test to search for Grapefruit and check install count text is "1 install". Also checks for Octopus module ("235 installs").

I used an xpath query to check that against the complete text on the element, as just checking for "1 install" in the output would also pass for "1 installs".

Test looks mostly good, but I'd rather it only test that the install formatting is good. It feels like it's also inadvertently testing search capabilities. Can we just make the first two results on page one our two test cases?

I realised that with a very small change to the fixture, we could get the module with 1 install onto the front page, simplifying the new test considerably.

Unfortunately, I ended up having to make a lot of small fixes to other tests. I think it's worth it, though.

I considered adding a "0 installs" test case, but I realised that when a module has no installs, there is no install count label at all. It might be worth adding a new test for that, but it seems out of scope for this issue. I will look at opening a follow on issue.

All tests are now passing, so moving to Needs Review.

Rebased for latest changes. Tests passing. Manually tested, and verified appearing correctly for modules with a single install.

Looks good to me. Tested manually and it is working as expected.
Should we also consider fixing 1 sites report using this module on modal in scope of this issue?

Moving it back to NW for #20, unless someone thinks otherwise.

I have reviewed the feedback mentioned in #20 and have tried to update the implementation accordingly, following the formatPlural same as for install case. Since the numberFormatter is not needed in this case, I have removed it. The pipeline has passed successfully, so I am moving this back to 'Needs Review.' Kindly review the changes.

I thin numberFormatter is required on both modal and listing page to display numbers formatted correctly.
Can we do something like:

        {Drupal.formatPlural(
          project.project_usage_total,
          `${numberFormatter.format(1)} site reports using this module`,
          `${numberFormatter.format(project.project_usage_total)} sites report using this module`
        )}

Sure , let me check.

Thank you, @narendrar, for your review and guidance. I have addressed the feedback and updated the test coverage accordingly. Moving this back to NR for your review. Kindly take a look at your convenience.

This looks goos to me, except that on token list/grid it is showing as 590949 Active Installs, but it should be 590,949 Active Installs, so this also needs to be changed accordingly.

Addressed the feedback for the install case as well. After applying the same approach for counting, the count is now displayed correctly, such as '590,949 Active Installs'. Kindly review.

This MR needs rebase and a change needs to be done at one more place.

I have rebased and resolved the conflicts in the MR. Additionally, I have updated the list view case as part of the changes. I realized I had missed updating the second instance of the same code earlier, which was highlighted in the feedback. This has now been corrected in both places. Kindly review the updated changes.

Changes looks good to me. Thanks for working on this issue. Moving it to RTBC.

This looks good. Leslie and I realized while manually testing, though it's going to be a follow-up, is that on the detail modal it's showing for 0 installs - it should be hiding altogether for 0 installs like on the cards. But again, that's a new issue.

chrisfromredfin → credited leslieg → .

thanks!