Problem/Motivation

When there is only 1 install of a module, it still says "1 installs" but proper grammar is "1 install"

Proposed resolution

We can use the Drupal.formatPlural JS method to format this better.

1 installs

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

chrisfromredfin created an issue. See original summary.

omkar-pd’s picture

Assigned: Unassigned » omkar-pd

omkar-pd’s picture

Assigned: omkar-pd » Unassigned
Status: Active » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

Needs to be rebased against 2.0.x...

omkar-pd’s picture

Status: Needs work » Needs review

Rebased

phenaproxima’s picture

Status: Needs review » Needs work

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

omkar-pd’s picture

Status: Needs work » Needs review

Required changes done.

phenaproxima’s picture

Status: Needs review » Needs work

Not quite done...

omkar-pd’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

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!

lostcarpark’s picture

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

chrisfromredfin’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
chrisfromredfin’s picture

Issue tags: +core-mvp, +beta blocker
lostcarpark’s picture

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".

lostcarpark’s picture

Status: Needs work » Needs review
chrisfromredfin’s picture

Status: Needs review » Needs work

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?

lostcarpark’s picture

Status: Needs work » Needs review

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.

lostcarpark’s picture

StatusFileSize
new49.54 KB

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

Screenshot of module with 1 install.

narendrar’s picture

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?

narendrar’s picture

Status: Needs review » Needs work

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

shalini_jha made their first commit to this issue’s fork.

shalini_jha’s picture

Status: Needs work » Needs review
StatusFileSize
new75.64 KB
new74.72 KB

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.

narendrar’s picture

Status: Needs review » Needs work

I think 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`
        )}
shalini_jha’s picture

Sure , let me check.

shalini_jha’s picture

Status: Needs work » Needs review

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.

narendrar’s picture

Status: Needs review » Needs work

This looks good 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.

shalini_jha’s picture

Status: Needs work » Needs review
StatusFileSize
new32.27 KB

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

shalini_jha’s picture

StatusFileSize
new34.78 KB
narendrar’s picture

Status: Needs review » Needs work

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

shalini_jha’s picture

Assigned: Unassigned » shalini_jha
shalini_jha’s picture

Assigned: shalini_jha » Unassigned
Status: Needs work » Needs review

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.

narendrar’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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

chrisfromredfin’s picture

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’s picture

chrisfromredfin’s picture

Status: Reviewed & tested by the community » Fixed

thanks!

Status: Fixed » Closed (fixed)

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