Challenge Details Page - Terms - Issues noted by reviewers and myself

[birdofpreyru]

• http://local.topcoder.com/challenges/30057879
  It will not show already agreed checkmarks/icon for completed/closed challenge
  This is an API issue: no way to check whether a user has agreed to a term not related to an active challenge. Will address it eventually, when terms are moved to API v3. For now it is not a reason to keep the issue open.

• you could use different color/style for Back button in term modal
  for example gray or red color

• http://local.topcoder.com/challenges/30058811
  Register directly without login it will still send register request with 401 response
  and it will not redirect to login page for such error case

• src\shared\services\terms.js
  It is not good to call get(/terms/${challengeId}?role=Submitter)
  directly and then
  this.private.api.get(/terms/${challengeId}?role=Submitter&noauth=true)
  you should check whether user has logon in and currently it will send two requests for not authenticated
  user

• src\shared\components\challenge-detail\Specification\SideBar\index.jsx

  <h3>DOWNLOADS:</h3>
        {
          hasRegistered && documents && documents.length > 0 ? (
  

  It is better to show DOWNLOADS: only if exist valid documents

• src\shared\actions\terms.js
  src\shared\reducers\terms.js
  src\shared\services\terms.js
  Not all functions has complete document

• src\shared\services\terms.js
  wrong document for terms service with tokenV2

  /**
  * Returns a new or existing challenges service.
  * @param {String} tokenV3 Optional. Auth token for Topcoder API v3.
  * @return {Challenges} Challenges service object
  */
  let lastInstance = null;
  export function getService(tokenV2) {
  

• shared/actions/terms.js

  /**
  * Payload creator for TERMS/GET_TERMS_INIT action,
  * which marks that we are about to fetch terms of the specified challenge.
  * If any challenge terms for another challenge are currently being fetched,
  * they will be silently discarded.
  * @param {Number|String} challengeId
  * @return {String}
  */
  function getTermsInit(challengeId) {
    return _.toString(challengeId);
  }
  

  Header comments must also describe the value being returned. You just specify that it's a string, but
  don't describe it

• shared/actions/terms.js

  /**
  * Payload creator for TERMS/GET_TERM_DETAILS_INIT action,
  * which marks that we are about to fetch details of the specified term.
  * If any details for another term are currently being fetched,
  * they will be silently discarded.
  * @param {Number|String} termId
  * @return {String}
  */
  function getTermDetailsInit(termId) {
    return _.toString(termId);
  }
  

  Similarly, you should put a description for parameters such as termId

• Missing header comments for functions

  function getDocuSignUrlInit(templateId) {
    return _.toString(templateId);
  }
  

• Missing header comment

  function getDocuSignUrlDone(templateId, returnUrl, tokenV2) {
    const service = getService(tokenV2);
    return service.getDocuSignUrl(templateId, returnUrl)
      .then(resp => ({ templateId, docuSignUrl: resp.recipientViewUrl }));
  }
  

• Missing header comment

  function agreeTermInit(termId) {
    return _.toString(termId);
  }
  
  function agreeTermDone(termId, tokenV2) {
    const service = getService(tokenV2);
    return service.agreeTerm(termId).then(resp => ({ termId, success: resp.success }));
  }
  

• Should add a header comment for this route

  app.use('/iframe-break', (req, res) => {
    const url = req.query.dest;
    res.send(`<script>window.top.location.href="${url}"</script>`);
  });
  

• server.js -> You add

  app.use('/iframe-break', (req, res) => {
    const url = req.query.dest;
    res.send(`<script>window.top.location.href="${url}"</script>`);
  });
  

  This is not a good way to redirect. You should be using redux - specifically use with the redux store of
  the parent window from the DocuSign iframe

• It looks from video, there are some problems with the DocuSign agreement flow (not refreshes the result automatically).

• Missing loading indicators when user waits for DocuSign to load and process agreement.

• Uses config.URL.LOCAL = local.topcoder.com, etc. to generate DocuSign return URL. It won't work when deployed remotely. Should use browser's location to generate return URL.