Closed (fixed)
Project:
Project Browser
Version:
2.0.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
27 Sep 2024 at 12:04 UTC
Updated:
24 Dec 2024 at 20:04 UTC
Jump to comment: Most recent, Most recent file

Comments
Comment #2
omkar-pd commentedComment #4
omkar-pd commentedComment #5
phenaproximaNeeds to be rebased against 2.0.x...
Comment #6
omkar-pd commentedRebased
Comment #7
phenaproximaOne question, otherwise I don't see any obvious problem with this.
Comment #8
omkar-pd commentedRequired changes done.
Comment #9
phenaproximaNot quite done...
Comment #10
omkar-pd commentedComment #11
phenaproximaThat looks correct to me - I think we'd want to use
project.project_usage_totalsinceformatPlural()is treating it as a number, not a string. RTBC assuming tests pass!Comment #12
lostcarpark commentedThe code change looks good, but should this have test coverage? Pretty sure if this was in core a test would be required.
Comment #13
chrisfromredfinComment #14
chrisfromredfinComment #15
lostcarpark commentedUpdated fixture to make the total installs of the Grapefruit module 1. Required some minor adjustments to existing tests.
Added
testInstallCountPluralFormattingtest 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".
Comment #16
lostcarpark commentedComment #17
chrisfromredfinTest 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?
Comment #18
lostcarpark commentedI 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.
Comment #19
lostcarpark commentedRebased for latest changes. Tests passing. Manually tested, and verified appearing correctly for modules with a single install.
Comment #20
narendrarLooks good to me. Tested manually and it is working as expected.
Should we also consider fixing
1 sites report using this moduleon modal in scope of this issue?Comment #21
narendrarMoving it back to NW for #20, unless someone thinks otherwise.
Comment #23
shalini_jha commentedI 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.
Comment #24
narendrarI think
numberFormatteris required on both modal and listing page to display numbers formatted correctly.Can we do something like:
Comment #25
shalini_jha commentedSure , let me check.
Comment #26
shalini_jha commentedThank 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.
Comment #27
narendrarThis looks good to me, except that on token list/grid it is showing as
590949 Active Installs, but it should be590,949 Active Installs, so this also needs to be changed accordingly.Comment #28
shalini_jha commentedAddressed 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.
Comment #29
shalini_jha commentedComment #30
narendrarThis MR needs rebase and a change needs to be done at one more place.
Comment #31
shalini_jha commentedComment #32
shalini_jha commentedI 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.
Comment #33
narendrarChanges looks good to me. Thanks for working on this issue. Moving it to RTBC.
Comment #34
chrisfromredfinThis 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.
Comment #37
chrisfromredfinComment #38
chrisfromredfinthanks!