Skip to content
Snippets Groups Projects

Resolve "When the branch name is a divider or separator, the project home page cannot display the branch name correctly."

2 unresolved threads

What does this MR do?

This MR fixes an issue where branches named separator or divider don't show up in the project ref switcher, but do cause it to render an actual divider or separator dropdown item.

Problem

The gl_dropdown.js file reads in string primitives for each data item. If these strings match either divider or separator, the file doesn't render the data, and instead renders an actual divider or separator item.

Unfortunately, "separator" and "divider" are much too easy to accidentally conflict with.

Solution

This MR completely refactors the rendering portion(s) of the gl_dropdown.js file.

All dividers and separators must be generated by { "type": ("separator"|"divider") } instead of plaintext strings.

More solution notes from commit c91f915a:


Basic Intent: Allow all branch names without accidentally creating layout or backstage DOM. e.g. a branch named separator should never create a separator li element.

Ideally, there should never be a string that could cause this kind of conflict.

Implementation: All of GitLabDropdown.renderItem is extracted to a standalone module.

To render a divider or separator, consumers must now pass in an object like { "type": "divider" } or { "type": "separator" }

Notable choices:

  • All of the functions have a cyclomatic complexity of 3 or less
    • See: https://en.wikipedia.org/wiki/Cyclomatic_complexity
    • Note the "Correlation to number of defects" section
    • While software complexity may not have a directly causal relationship with defects, less complex software is generally easier to reason about, and may reduce defects. I personally try to maintain complexity of no higher than 3.

Other Notes

There's an epic issue that addresses the double scrollbar problem that occurs when the branch/tag/commit list is scrollable, which is visible in the after screenshot (since the list is now long enough to scroll, barely), or on gitlab.com.

Screenshots

Before After
sep-before sep-after

Does this MR meet the acceptance criteria?

Conformity

Performance and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • ~~Label as security and @ mention @gitlab-com/gl-security/appsec~~
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Closes #65473 (moved)

Edited by Thomas Randolph

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Illya Klymov
  • Illya Klymov
  • Illya Klymov
  • Illya Klymov
  • Since we're altering the behavior of our component I think it's a good idea to have proper test coverage for it (it's our fault we didn't have proper tests before)

  • Illya Klymov
  • Illya Klymov
  • Illya Klymov
  • @thomasrandolph You've done some nice refactoring here! :tada:

    I generally agree with what Illya has pointed out so far, I would further emphasize:

    • We definitely need to add test coverage for this specific case (a dropdown entry named "divider" or "separator") to gl_dropdown_spec.js, and maybe review that spec while this is still fresh in your mind to make sure other dropdown options are covered properly
    • Some function names are not quite aligned with what the functions do and could cause confusion in future troubleshooting (hideRow(), handleOptions(), etc.)

    ... and I've opened one additional question/discussion :slight_smile:

  • unassigned @xanf

  • Thomas Randolph added 93 commits

    added 93 commits

    Compare with previous version

  • unassigned @andr3

  • Thomas Randolph added 33 commits

    added 33 commits

    • 1a0779a1...873a344e - 28 commits from branch master
    • 82fef293 - Extract and refactor dropdown item rendering
    • a02671a8 - Update tests for uncovered cases
    • 07bd37de - Update all uses of gl_dropdown to use objects
    • 7ebf9f99 - Remove unused variable
    • 10389ce8 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph changed the description

    changed the description

  • Thomas Randolph added 60 commits

    added 60 commits

    • 10389ce8...7698d405 - 55 commits from branch master
    • 8b785f7c - Extract and refactor dropdown item rendering
    • 1e32703d - Update tests for uncovered cases
    • bd33a6b9 - Update all uses of gl_dropdown to use objects
    • a7d3820c - Remove unused variable
    • e904279d - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 7 commits

    added 7 commits

    • e904279d...0aee1420 - 2 commits from branch master
    • 325bb4d5 - Extract and refactor dropdown item rendering
    • 4e884fc8 - Update tests for uncovered cases
    • 0e020535 - Update all uses of gl_dropdown to use objects
    • 9803563b - Remove unused variable
    • 25fc1972 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 7 commits

    added 7 commits

    • 25fc1972...ee18bf57 - 2 commits from branch master
    • 55b4b6ad - Extract and refactor dropdown item rendering
    • 093aa044 - Update tests for uncovered cases
    • 6e364050 - Update all uses of gl_dropdown to use objects
    • ea32bcd0 - Remove unused variable
    • 3ed63719 - Add changelog for fixing the ref switcher

    Compare with previous version

  • @xanf & @mfluharty Thank you for the great reviews!

    I've made some significant changes to the MR.

    Some of the notable things:

    • I moved all of the code that resolves the options + chunk properties to some value into a single map
      • A couple of them (getURL and getText) could share a generic function
      • Regardless of code re-use, I think this better groups things that resolve "something" from the chunk and "something" from the options into a single value
    • I added tests
      • Covers all special cases: separator, divider, and header
      • Also covers the new logic that short-circuits everything else if the item is hidden

    I think the changes address all of your other comments, but please let me know if anything is still amiss!

  • assigned to @xanf and @mfluharty and unassigned @thomasrandolph

  • Thomas Randolph added 13 commits

    added 13 commits

    • 3ed63719...e348def8 - 8 commits from branch master
    • e4a4bdf2 - Extract and refactor dropdown item rendering
    • 102e0ad9 - Update tests for uncovered cases
    • 5703e8e3 - Update all uses of gl_dropdown to use objects
    • ad09d1aa - Remove unused variable
    • debaaa86 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 77 commits

    added 77 commits

    • debaaa86...0fa2ea2e - 72 commits from branch master
    • 2924987e - Extract and refactor dropdown item rendering
    • 469b011a - Update tests for uncovered cases
    • 2fa787ef - Update all uses of gl_dropdown to use objects
    • 9fdbb5fb - Remove unused variable
    • f9ce0525 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph changed the description

    changed the description

  • Thomas Randolph added 9 commits

    added 9 commits

    • f9ce0525...663b7bb4 - 4 commits from branch master
    • 444d5a8c - Extract and refactor dropdown item rendering
    • f5a9960b - Update tests for uncovered cases
    • 0b9765db - Update all uses of gl_dropdown to use objects
    • 5d9a32c6 - Remove unused variable
    • 361bd380 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Illya Klymov
  • Illya Klymov
  • Illya Klymov
  • @thomasrandolph Just a few more concerns, looks great!

  • Illya Klymov assigned to @thomasrandolph and unassigned @xanf

    assigned to @thomasrandolph and unassigned @xanf

  • Thomas Randolph added 216 commits

    added 216 commits

    • 361bd380...4bf01ce5 - 211 commits from branch master
    • 509b2bab - Extract and refactor dropdown item rendering
    • 6ca9003e - Update tests for uncovered cases
    • faf2e50e - Update all uses of gl_dropdown to use objects
    • cf91eb17 - Remove unused variable
    • d03c660e - Add changelog for fixing the ref switcher

    Compare with previous version

  • assigned to @xanf and unassigned @mfluharty and @thomasrandolph

  • Thomas Randolph resolved all threads

    resolved all threads

  • Thomas Randolph changed the description

    changed the description

  • Thomas Randolph added 10 commits

    added 10 commits

    • d03c660e...b23e42b0 - 5 commits from branch master
    • 77ba850c - Extract and refactor dropdown item rendering
    • 37e12b3e - Update tests for uncovered cases
    • 30468b3b - Update all uses of gl_dropdown to use objects
    • abb796ab - Remove unused variable
    • db43afed - Add changelog for fixing the ref switcher

    Compare with previous version

  • Illya Klymov approved this merge request

    approved this merge request

  • assigned to @pslaughter and unassigned @xanf and @thomasrandolph

  • @pslaughter Looking for a review / merge!

  • Thomas Randolph resolved all threads

    resolved all threads

  • Thomas Randolph added 117 commits

    added 117 commits

    • db43afed...a15a69cf - 112 commits from branch master
    • c91f915a - Extract and refactor dropdown item rendering
    • da2ab7ac - Update tests for uncovered cases
    • 241c5938 - Update all uses of gl_dropdown to use objects
    • fec35632 - Remove unused variable
    • cb29dd9d - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph changed the description

    changed the description

  • Thomas Randolph resolved all threads

    resolved all threads

    • issue: So... Your MR and I went on a little adventure together :smile:

      adventure time

      Wait! Why?

      In case you haven't noticed, gl_dropdown is used in a lot of places...

      git grep -e 'glDropdown' | wc -l
      
      75

      Also, as you're well aware, gl_dropdown isn't pretty (I think it's a relic from the ol' coffeescript days), so changes to it have a high probability of introducing bugs. (By the way, I love how you isolated your change to this one wild function!)

      Long story short, if we want to make a refactor like this, we need to be very very sure it doesn't introduce bugs (oh yeah... and modules like this don't have the best testing coverage either)...

      Okay Paul, so how do we make sure it doesn't introduce bugs when it's largely untested?

      With a Pinning Test!

      (shoutout to a very insightful and frightening book called Working Effectively with Legacy Code)

      So lets create a pin of GitLabDropdown.renderData using Jest snapshots. In the following commit, I created a snapshot for each significant permutation of input and state after analyzing renderItem and renderData:

      After applying this MR ontop of that branch and updating the spec to match the new API, there were a few changes in the snapshots:

      • The order of is-active is different

      Screen_Shot_2019-09-05_at_2.39.55_AM

      • The original renders the element even if hiding

      Screen_Shot_2019-09-05_at_2.38.48_AM

      • The original allowed passing gl_dropdown instance to renderRow (most significant)

      Screen_Shot_2019-09-05_at_2.41.31_AM

      Here's a patch to fix these issues:

      0001-Fix-gl_dropdown-render-failing-snapshots.patch

      Could you please review and apply this patch? This way we can assuredly say that this MR is a pure refactor except for the API bug fix. Thanks! :bow:

    • Thank you very much for this patch!

      In my estimation, it's A+ and I applied it.

      That said:

      1. A previous review suggested short-circuiting the render if the item was hidden, and I agreed - it seems wasteful
      2. A codebase search for the instance variable being passed to the arbitrary renderRow function returned only one result... and the variable was not used in the body of that function.

      Between these items and the ordering of the CSS classes, I have a question: how frequently might a change like the order of CSS classes or removing an unused function parameter cause unintended side-effects? Or - put another way - how much consternation should I have in the future about seemingly "trivial" changes like the order of a class list?

      P.S. I can totally imagine how some other JS might expect hidden items to be rendered, so that one makes sense to me, but it bums me out :)

    • A previous review suggested short-circuiting the render if the item was hidden, and I agreed - it seems wasteful

      I agree. It's wasteful :/

      Can we possible address this in a follow up MR?

      A codebase search for the instance variable being passed to the arbitrary renderRow function returned only one result... and the variable was not used in the body of that function.

      Good investigation!

      how frequently might a change like the order of CSS classes or removing an unused function parameter cause unintended side-effects?

      Probably never, but stranger things have happened :smile:

      Or - put another way - how much consternation should I have in the future about seemingly "trivial" changes like the order of a class list?

      This is a good question. The reason I'm being hyper detailed about this is that these old classes are notoriously fragile and used all over the place. It's risky to add behavior, or refactor. It's even more risky to add behavior and refactor. We don't want bug fix MR's to be reverted because a refactoring introduced another issue.

      All of this to say, I prefer to be very explicit that a refactor is a pure refactor with no unintended side-effects. I think the pinning testing technique gives us confidence in this. If the pinning test fails, I get nervous :sweat_smile:

      Sometimes this isn't always possible to refactor without introducing trivial side-effects, but in this case I think it is avoidable and preferred. :shrug:

    • Please register or sign in to reply
  • Paul Slaughter
  • Thanks @thomasrandolph for braving to refactor and simplify some of this legacy code! Changes to these kinds of site-wide modules make me nervous, and even after all the checking, there's a risk that this introduces some bugs. On the other hand, there's a huge benefit with us untangling some of this so that it will be easier to translate to Vue in the future :tada:

    I left some comments and patches. I'd recommend checking out these two first:

    Back to you! :soccer:

  • assigned to @thomasrandolph and unassigned @pslaughter

  • Thomas Randolph added 66 commits

    added 66 commits

    • cb29dd9d...f7e7ee71 - 61 commits from branch master
    • f4bda732 - Extract and refactor dropdown item rendering
    • 19016b0f - Update tests for uncovered cases
    • 69671673 - Update all uses of gl_dropdown to use objects
    • e293b4a6 - Remove unused variable
    • 4878e3df - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 5 commits

    added 5 commits

    • c2f0882b - Extract and refactor dropdown item rendering
    • 78bb7212 - Update tests for uncovered cases
    • 08289bb5 - Update all uses of gl_dropdown to use objects
    • 588bdc0d - Remove unused variable
    • d7dea643 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 5 commits

    added 5 commits

    • d8ab43a9 - Extract and refactor dropdown item rendering
    • 67166a56 - Update tests for uncovered cases
    • efea2458 - Update all uses of gl_dropdown to use objects
    • 9b097757 - Remove unused variable
    • ef65d683 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 5 commits

    added 5 commits

    • ed1966c4 - Extract and refactor dropdown item rendering
    • 505e9a6f - Update tests for uncovered cases
    • d2e6a939 - Update all uses of gl_dropdown to use objects
    • 34418f75 - Remove unused variable
    • 20fda604 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 5 commits

    added 5 commits

    • 0e228d91 - Extract and refactor dropdown item rendering
    • 661ae1b9 - Update tests for uncovered cases
    • 44f30225 - Update all uses of gl_dropdown to use objects
    • c34bf8c3 - Remove unused variable
    • 454c9748 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph added 7 commits

    added 7 commits

    • 454c9748...1efb4ea8 - 2 commits from branch master
    • 14038a24 - Extract and refactor dropdown item rendering
    • a95460bf - Update tests for uncovered cases
    • 079d6496 - Update all uses of gl_dropdown to use objects
    • b0ded03e - Remove unused variable
    • eb346616 - Add changelog for fixing the ref switcher

    Compare with previous version

  • Thomas Randolph changed the description

    changed the description

  • Thomas Randolph resolved all threads

    resolved all threads

  • @pslaughter Back to you! I've applied all your recommendations and patches, and left a few comments. Only one of which actually raises a question, but the others may spark some conversation :)

    I've also assigned the corollary EE MR to you, since it's a dependency.

  • assigned to @pslaughter and unassigned @thomasrandolph

  • update: @thomasrandolph, thanks a lot for working on this! I'm excited about the refactor and the fix and I appreciate you letting me be nitpicky about keeping the refactor as pure as possible.

    I'm 80% certain this MR is good to go. I'd like to test it locally once more, but I won't be able to do that until tomorrow. Stay tuned! :tv:

  • Thomas Randolph added 121 commits

    added 121 commits

    • eb346616...fd515cca - 116 commits from branch master
    • 16ecdf2a - Extract and refactor dropdown item rendering
    • 817a36d0 - Update tests for uncovered cases
    • 6de1bc72 - Update all uses of gl_dropdown to use objects
    • 5eafdc31 - Remove unused variable
    • 814ff26c - Add changelog for fixing the ref switcher

    Compare with previous version

    • Hey! :wave:

      GitLab is moving all development for both GitLab Community Edition and Enterprise Edition into a single codebase. The current gitlab-ce repository will become a read-only mirror, without any proprietary code. All development is moved to the current gitlab-ee repository, which we will rename to just gitlab in the coming weeks.

      As part of this migration, issues will be moved to the current gitlab-ee project. Merge requests can not be moved, and will be closed and locked instead.

      On September 22, 2019 we will start closing all remaining CE merge requests. We have also disabled merging of merge requests in CE.

      Here is what you should do to make sure your changes are merged before then:

      1. Continue the development process as usual.
      2. When the merge request has been reviewed, create a new merge request with these changes at https://gitlab.com/gitlab-org/gitlab-ee/merge_requests.
      3. In this new merge request, mention the old merge request.
      4. Close the old merge request, and in a comment mention the new merge request.
      5. Assign the merge request to the person who last reviewed it, or @ mention them if you are not able to assign the merge request to them.

      When creating your EE merge request, you can follow these steps to port over your changes:

      1. Add your local copy of GitLab CE as a Git remote to your copy of GitLab EE. This is done using git remote add local-ce path/to/ce/.git.
      2. Run git fetch local-ce BRANCH, where BRANCH is the branch containing your CE changes.
      3. For every commit in your CE MR, run git cherry-pick COMMIT, with COMMIT being the commit SHA. Squashing your CE commits before may make this process easier.
      4. Push these commits to a branch of fork like you normally would when submitting your changes.
      5. Submit these changes as a merge request at https://gitlab.com/gitlab-org/gitlab-ee/merge_requests.

      More information about this can be found here: https://docs.gitlab.com/ee/development/automatic_ce_ee_merge.html#cherry-picking-from-ce-to-ee

      If you have any questions about all of this, please ask them in our dedicated FAQ issue: https://gitlab.com/gitlab-org/gitlab-ee/issues/13855

      You can also find more information here:

  • I'm going to close this MR in favor of the EE port (which refers back to this one for the writeup/discussion): https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/16165

  • assigned to @thomasrandolph and unassigned @pslaughter

  • mentioned in merge request gitlab!16165 (merged)

  • mentioned in merge request gitlab!28330 (merged)

  • Please register or sign in to reply
    Loading