Community Terms - issues noted during review

[birdofpreyru]

• We should figure out why terms statuses are not consistent in the backend! See this discussion from review and appeals:
  Response 1: Comment (talesforce)
  When the user has not even logged in, why do you set agreed to 'true' - it should be set to 'false' in this
  case
  Response 1: Recommended (talesforce)
  Submission uses a nocache random value param in getTermDetails, this is hacky way of doing things
  and should be approved by copilot in forums before implementation
  Appeal: Response 2
  Probably I wasn't clear about this. It's not that I set it but server strangely returns true in this case, and I
  wanted to point it out so probably copilot can note it and discuss with backend guys about fixing it or at
  least be aware. Also, I've noted it in submission.md in implementation notes "Though when user is not
  logged in, terms details responses with agreed=true!"

  Response 3
  I didn't ask copilot as I'm aware of such server issues since another challenge where the same problem
  take place for the challenge terms https://www.topcoder.com/challenge-details/30059492/?type=develop
  I've fixed it for challenge terms before but that fix doesn't suite community terms well, so I've made
  another one which is even better and I advised in the implementation notes to try to apply the same fix
  for challenge terms.

  So in general, these are server issues and I'm just finding workarounds to work with them. Please don't
  score down here as copilot is generally aware of such issues.

• src/shared/components/Terms/styles.scss
  color: #000;

  this should use $tc-black constant for the black color

• src/shared/containers/tc-communities/JoinCommunity.jsx
  <JoinCommunity {...{ ...props, join: onJoinClick }} />

  This can be re-written in the better (and more efficient way, as avoids
  creation of intermediate JS object on the way):
  <Join Community {...props} join={onJoinClick} />

• src/shared/containers/Terms.jsx
  The way you implemented it, it renders its content only when
  showTermsModal is passed in with props. For my taste, it would be better
  to move this check out of this container, leaving it up to parent to
  decide whether to render entire container or not.

• src/shared/actions/terms.js
  I don't really like that some stuff related to mock-up functionality
  (mockAgreed params) shows up around the common code. My concern is that
  if we start doing it in different places, it is easy to eventually mix up
  the things and get some mock functionality leaked into production use.

• When, with MOCK_TERMS_SERVICE on, I try to join community not-protected
  by any terms, it still shows me the terms. I know, it just follows the
  logic how the mock was working with the challenge terms, but in this
  new use case it seems confusing to me. Mostly because when I test
  something related to challenges, they have been created in Direct,
  I don't really care what terms have been set there, cool. But with
  communities, at the moment, we configure the terms inside our Community App
  code, so having the mock working this way does not allow to verify our
  configuration is correct.

• docs/how-to-add-a-new-topcoder-community.md
  In addition to docs you have updated, we have this document about
  configuration of Topcoder communities. It was necessary to add a note
  about terms array in there as well.

• In the following method:

  /**
  * get all terms for community
  *
  * NOTE: As there is no specific endpoint to get community terms by one call
  *      currently we get community term ids from community service and after
  *      we get community terms using term ids list one by one
  *
  * @param {String} communityId community id
  * @param {String} tokenV3    auth token V3 - we need to get community meta data
  *
  * @return {Promise} resolves to the list of community terms
  */
  getCommunityTerms(communityId, tokenV3) {
    const communityService = getCommunityService(tokenV3);
  
    return communityService.getMetadata(communityId).then((meta) => {
      if (meta.terms && meta.terms.length) {
        return Promise.all(meta.terms.map(termId => this.getTermDetails(termId))).then(terms => (
          terms.map(term => _.omit(term, 'text')) // don't include text as it's big and we need it for list
        ));
      }
  
      return [];
    }).then(terms => ({
      terms,
    }));
  }

  Should return an error in case of failure to be consistent with other existing APIs

• reducer/terms.js
  const selectedTerm = _.find(state.terms, t => !t.agreed) || state.terms[0];
  You should have an inline comment describing what the 0th index of terms holds

• // if it's commynity page
  let communityId = getCommunityId(req.subdomains);
  Typo - community

• joinCommunity.jsx
  function mapStateToProps(state, ownProps) {
  Should include header comments describing method, params, return value

• joinCommunity.jsx

  function mapDispatchToProps(dispatch) {
  const a = actions.tcCommunity;
  const t = termsActions.terms;
  return {
  

  Should include header comments describing method, params, return value

• // this is currently never happens that we have mounted Terms component -> // this will currently never
  happen that we mounted Terms component

• TermDetail.jsx - Should include a header comment for file explaining the purpose of this jsx file

• index.jsx -
  function handleScroll(scrollElement, masks, orientation) {
  let length;
  let base;
  let clientSize;

  Should include header comments describing method, params, return value