[#1532] Change mergegroups in URL to be group index for concision

[jonasongg]

Fixes #1532

Proposed commit message

Change mergegroup hash in URL to refer to group index instead of name

Currently, the mergegroup hash param in the URL lists down all groups
that are merged. This can result in an extremely long URL when many
groups are merged which can cause the URL to exceed the length limit.

Let's change the param to refer to the repo index instead to prevent
this issue.

Other information

This required a few changes to the order in which methods are called in created() of c-summary.vue. The original behaviour was renderFilterHash sets data from the URL, then getFiltered sets the URL from the fields as well as populates this.filtered. Changing the URL to refer to group index instead necessitates that setting and getting the merged groups from the URL happen after this.filtered is populated.

It might not be worth changing the logic of c-summary.vue just to shorten the URL; let me know what you think! If it's worth it to make this change, I will need to change codeView_renderFilterHash.cy.js as well. (So please ignore the failing CI for now!)

An additional consideration is to +1 the index that is displayed in the URL to match the index shown on the page since arrays are zero-indexed but the index shown on the page is one-indexed.

[georgetayqy]

Looks good! I have rerun some failed jobs and managed to get some of them to pass CI. However, some failing frontend test cases have caused CI to fail, specifically:

Running:  chartView/chartView_zoomFeature.cy.js                                          (9 of 25)


  zoom features in code view
    ✓ click on view commits button (1035ms)
    ✓ zoom into ramp (482ms)
    ✓ zoom into ramp when merge group (595ms)

  date changes in chart view should reflect in zoom
    ✓ setting 'since' date range changes the zoom view (1134ms)
    ✓ setting 'since' date again results in a different zoom view (976ms)
    ✓ setting the 'until' date changes the zoom view (1187ms)
    ✓ setting the 'until' date again results in a different zoom view (1435ms)
    ✓ setting the 'until' and 'since' date changes the zoom view (1316ms)
    ✓ setting the 'until' and 'since' date again results in a different zoom view (1329ms)

  range changes in chartview should reflect in zoom
    1) selecting the initial righthand and lefthand boundary
    2) changing the righthand boundary
    ✓ changing the lefthand boundary (1387ms)
    ✓ changing the righthand and lefthand boundary (806ms)


  11 passing (1m)
  2 failing

  1) range changes in chartview should reflect in zoom
       selecting the initial righthand and lefthand boundary:

      Timed out retrying after 30000ms
      + expected - actual

      -'[2019-12-31] [#785] ChartView: fix authors with unaccounted lines (#789): +49 -42 lines '
      +'[2019-12-20] [#46] Show total time after batch processing (#758): +43 -0 lines '
      
      at Context.eval (webpack:///./tests/chartView/chartView_zoomFeature.cy.js:270:7)

  2) range changes in chartview should reflect in zoom
       changing the righthand boundary:

      Timed out retrying after 30000ms
      + expected - actual

      -'[2019-12-31] [#785] ChartView: fix authors with unaccounted lines (#789): +49 -42 lines '
      +'[2019-12-20] [#46] Show total time after batch processing (#758): +43 -0 lines '
      
      at Context.eval (webpack:///./tests/chartView/chartView_zoomFeature.cy.js:296:7)




  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        13                                                                               │
  │ Passing:      11                                                                               │
  │ Failing:      2                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      0                                                                                │
  │ Screenshots:  0                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     1 minute, 14 seconds                                                             │
  │ Spec Ran:     chartView/chartView_zoomFeature.cy.js                                            │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘

Running:  codeView/codeView_renderFilterHash.cy.js                                      (17 of 25)


  render filter hash
    ✓ search: url params should persist after change and reload (1340ms)
    ✓ group by: url params should persist after change and reload (888ms)
    ✓ sort groups by: url params should persist after change and reload (1611ms)
    ✓ sort within groups by: url params should persist after change and reload (1613ms)
    ✓ granularity: url params should persist after change and reload (670ms)
    ✓ since: url params should persist after change and reload (647ms)
    ✓ until: url params should persist after change and reload (708ms)
    ✓ breakdown by file type: url params should persist after change and reload (348ms)
    1) merge all groups: url params should persist after change and reload
    ✓ checked file types: url params should persist after change and reload (1265ms)


  9 passing (40s)
  1 failing

  1) render filter hash
       merge all groups: url params should persist after change and reload:
     AssertionError: Timed out retrying after 30000ms: expected 'http://localhost:9000/?search=&sort=groupTitle%20dsc&sortWithin=title&since=2018-05-03&timeframe=commit&groupSelect=groupByRepos&breakdown=false&mergegroup=0' to include 'mergegroup=reposense%2FRepoSense%5Bcypress%5D'
      at Context.eval (webpack:///./tests/codeView/codeView_renderFilterHash.cy.js:297:7)




  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        10                                                                               │
  │ Passing:      9                                                                                │
  │ Failing:      1                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      0                                                                                │
  │ Screenshots:  0                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     39 seconds                                                                       │
  │ Spec Ran:     codeView/codeView_renderFilterHash.cy.js                                         │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘

[jonasongg]

@asdfghjkxd Thanks for the review! I originally wanted to see if it was worth performing this change before I changed the cypress tests, but with your review, I'll go ahead and change it :)

[ckcherry23]

Hi @jonasongg, noticed a bug in the current implementation:

Steps to reproduce

1. Merge the first and second repositories using the Merge Group icon. Let's say their names are A and B respectively.
2. Change the sort by option to something else, for example, '↑ Contribution'.

Expected

1. A and B are still merged when the sort order is changed.
2. The rest of the repos are not merged.

Actual

1. Instead of A and B being the merged ones, the first and second repositories in the new order are the merged groups.

---

Can you please take a look?

[jonasongg]

@ckcherry23 Should be fixed now!

[jonasongg]

@ckcherry23 Also added a test for this bug!

[ckcherry23]

Although the code seems to work well and partially fixes the long URLs issue, I feel like the new code structure of adding/encoding hashes is not simple to understand anymore. Is there anything we can do to make the code more readable?

[github-actions]

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

[github-actions]

This PR was closed because it has been marked as stale for 7 days with no activity.
Feel free to reopen this PR if you would like to continue working on it.