Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include version number in issues created #656

Merged
merged 6 commits into from
Mar 24, 2021

Conversation

dingyuchen
Copy link
Contributor

@dingyuchen dingyuchen commented Mar 19, 2021

Summary

This solves issue #655.
This change involves making isElectron() static in order to detect if the app is running as the Web version or Desktop application.

Proposed Commit Message

We don't have a good way of detecting which version of CATcher students are using.

Let's
* Include the CATcher version number in issues created
* Include the CATcher client being used (Desktop or Web)

Version comment will be in the form "Version: {Web|Desktop} vX.X.X".
For example:
- <!--Version: Web v3.3.7-->
- <!--Version: Desktop v3.3.6-->

@dingyuchen dingyuchen requested review from anubh-v and a team March 19, 2021 02:38
@damithc
Copy link
Contributor

damithc commented Mar 19, 2021

Thanks for the quick PR @dingyuchen 👍
BTW, best to let others know before starting work on a PR (to prevent duplication of work).

Looks like this part is not yet covered by tests? I was searching among the PR diff to see any changes to test cases, so that I can get an idea of the change to the external behavior. Couldn't find any.

@dingyuchen
Copy link
Contributor Author

dingyuchen commented Mar 19, 2021

Regarding isElectron() and currentVersion, perhaps it will be good if the functions / variables in question are moved to a module in the shared library, that other services will reference. This should let us pull pertinent data without instantiating an entire service object.

Since this would affect other ongoing PRs, I shall open a new issue for this.
For this PR, I'll revert the refactoring of isElectron() just to keep the changed files small, and make another PR abstracting those functionality once #659 is merged. Is that ok? @ptvrajsk @seanlowjk @damithc

@anubh-v
Copy link
Contributor

anubh-v commented Mar 20, 2021

Regarding isElectron() and currentVersion, perhaps it will be good if the functions / variables in question are moved to a module in the shared library, that other services will reference. This should let us pull pertinent data without instantiating an entire service object.

I agree that it will be good for currentVersion. We can still keep the variable within the ApplicationService file (instead of a new module), as it seems most closely related to this service.

For isElectron(), we could also move it to a standalone function that's exported independently from ElectronService.
In my view, the benefit of this will be limited though, as most services / components that require ElectronService also use its other methods (which have state).

For this PR, I'll revert the refactoring of isElectron() just to keep the changed files small,

Edit: Let's shift isElectron separately, as it potentially affects more areas of the code.
We can move the currentVersion variable in this PR.

Copy link
Contributor

@anubh-v anubh-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work @dingyuchen .
Can you also update the HiddenData tests, such that they use multiple hidden comments (two is fine) ?

src/app/core/services/issue.service.ts Outdated Show resolved Hide resolved
src/app/core/services/issue.service.ts Outdated Show resolved Hide resolved
tests/services/logging.service.spec.ts Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 21, 2021

Codecov Report

Merging #656 (1405682) into master (ed6f72e) will increase coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
+ Coverage   67.68%   67.71%   +0.02%     
==========================================
  Files          75       75              
  Lines        2259     2264       +5     
  Branches      207      208       +1     
==========================================
+ Hits         1529     1533       +4     
- Misses        686      687       +1     
  Partials       44       44              
Impacted Files Coverage Δ
src/app/core/services/issue.service.ts 31.00% <60.00%> (+0.89%) ⬆️
src/app/core/services/application.service.ts 91.17% <100.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed6f72e...1405682. Read the comment docs.

@dingyuchen dingyuchen requested a review from anubh-v March 21, 2021 10:29
@anubh-v anubh-v added this to the V3.3.8 milestone Mar 23, 2021
@anubh-v
Copy link
Contributor

anubh-v commented Mar 23, 2021

@dingyuchen can you include examples of the HTML comments (for both versions) in the commit message?

@anubh-v
Copy link
Contributor

anubh-v commented Mar 23, 2021

@CATcher-org/2021-devs any further comments?

@damithc
Copy link
Contributor

damithc commented Mar 23, 2021

@dingyuchen I need to know exactly how the body of the issue will change due to this PR, if it does change the body at all. My scripts depends on the issues following a specific format.

Copy link
Contributor

@seanlowjk seanlowjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CATcher-org/2021-devs any further comments?

No comments from me!

@dingyuchen
Copy link
Contributor Author

dingyuchen commented Mar 23, 2021

@dingyuchen I need to know exactly how the body of the issue will change due to this PR, if it does change the body at all. My scripts depends on the issues following a specific format.

@damithc Oh sflr!
The version number is added as a html comment after the session data as such:
image

I'll update the commit message to include an example

@damithc
Copy link
Contributor

damithc commented Mar 23, 2021

The version number is added as a html comment after the session data as such:
image

Got it. Should be fine.

@anubh-v anubh-v merged commit 5d068dc into CATcher-org:master Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants