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

Member archives #282

Merged
merged 9 commits into from
Aug 14, 2018
Merged

Member archives #282

merged 9 commits into from
Aug 14, 2018

Conversation

Embraser01
Copy link
Member

@Embraser01 Embraser01 commented Aug 12, 2018

Closes #230.

Description of your PR

Add a new model for registration. This means that we don't really need a archive system, just filter per year

Pre-review TODO

  • PR is rebased on top of origin/master
  • PR provide new tests for new behaviors
  • PR doesn't introduce commented out code
  • PR has been tested locally on the main use cases
  • Documentation have been updated if necessary

More details about this PR

@codecov
Copy link

codecov bot commented Aug 12, 2018

Codecov Report

Merging #282 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #282   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          78     79    +1     
  Lines        1653   1697   +44     
  Branches      172    179    +7     
=====================================
- Misses       1481   1518   +37     
- Partials      172    179    +7
Impacted Files Coverage Δ
server/bootstrap/sequelize.js 0% <ø> (ø) ⬆️
server/app/services/permission-service.js 0% <0%> (ø) ⬆️
server/app/services/special-account-service.js 0% <0%> (ø) ⬆️
server/app/controllers/me-controller.js 0% <0%> (ø) ⬆️
server/app/services/recaptcha-service.js 0% <0%> (ø) ⬆️
server/app/services/feed-service.js 0% <0%> (ø) ⬆️
server/app/services/barman-service.js 0% <0%> (ø) ⬆️
server/app/controllers/feed-object-controller.js 0% <0%> (ø) ⬆️
server/app/routes/templates.js 0% <0%> (ø) ⬆️
server/app/services/task-service.js 0% <0%> (ø) ⬆️
... and 19 more

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 1b81187...7098d33. Read the comment docs.

@Embraser01 Embraser01 added front business Everything related to business and removed Do not merge labels Aug 12, 2018
Copy link

@Botweiser Botweiser left a comment

Choose a reason for hiding this comment

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

SonarQube analysis found issues:
Bug Bugs: 0
Vulnerability Vulnerabilities: 0
Code Smell Code Smells: 2

See all issues in SonarCloud

Copy link
Member

@corentingiraud corentingiraud left a comment

Choose a reason for hiding this comment

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

Do you update swagger & draw.io model diagram ?

@@ -32,7 +33,7 @@ export class MembersListComponent implements OnInit {
ngOnInit(): void {
this.update();
if (!this.ngxPermissionsService.getPermissions()['member:write']) {
this.displayedColumns = ['lastName', 'firstName', 'school'];
this.displayedColumns = ['lastName', 'firstName', 'lastActive', 'school'];
}
this.media.subscribe((change: MediaChange) => {
if (change.mqAlias === 'xs' && this.displayedColumns.includes('school')) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want that lastActive column is displayed on mobile device ? I don't think so !

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, for now I would like to let it here, because its the only way to see if a member is active or not. We could remove it the day we do #285

Copy link
Member

Choose a reason for hiding this comment

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

So on mobile device, there is an issue :)
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, do you have an idea to fix this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes :) I'm going to commit a fix

Copy link
Member

Choose a reason for hiding this comment

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

image

@Embraser01
Copy link
Member Author

I did not update swagger nor draw.io, feel free to do so, otherwise I will do it this evening 😉

@corentingiraud corentingiraud merged commit 52c0670 into master Aug 14, 2018
@corentingiraud corentingiraud deleted the feature/230-Members-archive branch August 14, 2018 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
business Everything related to business front
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants