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

Issue #49: Granting Stock Keepers access to ALTA through the mobile client #165

Merged
merged 14 commits into from
Nov 30, 2020

Conversation

makchamp
Copy link
Contributor

@makchamp makchamp commented Nov 28, 2020

New API endpoint to login from the mobile client

  • New 'login-mobile' endpoint will handle logging in exclusively from the mobile client
    This new endpoint will distinguish from which client the user is logging in from
    For now the authentication logic is still exactly the same (it will be changed later, as the authentication process has different requirements for stock keepers see: User Story 4.9 User Story 4.9 #132)

Current issues are mainly going to be related to routing. A big head scratcher is finding a way to keep the routing between the mobile and web client separate. This is a big deal because the default route is automatically redirected to after the app is reopened after being closed (so the web dashboard will be shown as that is set as being redirected from the default route). Also we potentially might want these mobile routes to not be accessible from the normal browser.

A nice cleaner option would be to use a multi-project workspace:https://angular.io/guide/file-structure#multiple-projects

Edit:
I have tried to implement the multiple project solution with some success but the drawbacks were too severe to go ahead with the solution. This is mainly due to ionic still requiring the entire config files (like its own package.json) to be generated, unless the command 'ionic init' is used which transforms an existing angular project into an ionic one.
However, when attempting this, porting the current web application to the multi project structure causes weird UI bugs to surface.
It would be possible to look into this later but for now I determined it to be not worth the time effort.

I decided to simply generate a full isolated ionic app. This has the major benefit of ease of use and maintainability as the ionic specific code will be fully segregated. Also i recon performance and device compatibility will be improved as ionic specific elements are used everywhere. The main drawbacks is that there might be some minor code redundancies and a less elegant directory structure.

The ionic app (''alta-mobile') now includes:

  • Fully ported authentication and guards
  • Implementing canLoad to secure the routes
  • Log in form
  • Side menu navigation

From inside the client/alta-mobile directory there is no need to specify the project name anymore to run any of the ionic commands:
For example:
To start the app:
ionic serve
To add changes to native builds:
ionic cap copy

New 'login-mobile' endpoint will handle logging in exclusively from the mobile client
This new endpoint will distinguish which client the user is logging in from
For now the authentication logic is still exactly the same (it will be changed later, as the authentication process has different requirements for stock keepers)
However only stock keepers (and inventory managers) will currently be redirected to the mobile landing page
We require seperate routing for the mobile app
Landing 'activity' after mobile log in
Using an ion-menu as the side navigation drawer
@makchamp makchamp changed the title Issue #49: Granting Stock Keeper acces to ALTA throught the mobile client Issue #49: Granting Stock Keeper acces to ALTA through the mobile client Nov 28, 2020
@github-actions
Copy link

Pylint Report

'- -----------------------------------
'- Your code has been rated at 9.95/10+ ------------------------------------------------------------------
'+ Your code has been rated at 9.95/10 (previous run: 9.95/10, +0.00)

@github-actions
Copy link

NG Lint Report

@makchamp makchamp changed the title Issue #49: Granting Stock Keeper acces to ALTA through the mobile client Issue #49: Granting Stock Keepers acces sto ALTA through the mobile client Nov 28, 2020
@makchamp makchamp changed the title Issue #49: Granting Stock Keepers acces sto ALTA through the mobile client Issue #49: Granting Stock Keepers access to ALTA through the mobile client Nov 28, 2020
A standalone app is alot more easily maintainable
I have attempted many multi-project solutions with contstant issues.
The drawback of this solution is slightly increased code redundancy
This is a template app generated by ionic start with the sidenav option
The template code required some changes to fit our requirements
Added 3 template pages to later rename and use for the side menu
Routing is all attached to it
Integrated authetnicated login with tokens and auth service
Protected routes
login by email and password
Using the canLoad interface for protected pages
Automatically closing the nav menu when a user clicks on a route
Making the size of it smaller
Since the ionic app is kept seperate now, this code is no longer required
@github-actions
Copy link

Pylint Report

'+ ************* Module user_account.urls
'+ user_account/urls.py:18:5: E0001: invalid syntax (, line 18) (syntax-error)
'+ user_account/views.py:175:0: C0301: Line too long (101/100) (line-too-long)
'+ user_account/views.py:176:0: C0303: Trailing whitespace (trailing-whitespace)
'+ user_account/views.py:174:2: W0511: TODO: Implement mobile specific endpoint (fixme)
'- -----------------------------------
'- Your code has been rated at 9.95/10+ ------------------------------------------------------------------
'+ Your code has been rated at 9.80/10 (previous run: 9.95/10, -0.14)

@github-actions
Copy link

NG Lint Report

@github-actions
Copy link

Pylint Report

'+ ************* Module user_account.urls
'+ user_account/urls.py:18:86: C0303: Trailing whitespace (trailing-whitespace)
'+ user_account/urls.py:18:2: W0511: TODO: Call mobile specific view (fixme)
'+ user_account/views.py:175:0: C0301: Line too long (101/100) (line-too-long)
'+ user_account/views.py:176:0: C0303: Trailing whitespace (trailing-whitespace)
'+ user_account/views.py:174:2: W0511: TODO: Implement mobile specific endpoint (fixme)
'- -----------------------------------
'- Your code has been rated at 9.95/10+ ------------------------------------------------------------------
'+ Your code has been rated at 9.86/10 (previous run: 9.95/10, -0.09)

@github-actions
Copy link

NG Lint Report

@codecov-io
Copy link

codecov-io commented Nov 29, 2020

Codecov Report

Merging #165 (b02cade) into master (7a71231) will decrease coverage by 0.12%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   97.40%   97.28%   -0.13%     
==========================================
  Files          20       20              
  Lines         501      515      +14     
==========================================
+ Hits          488      501      +13     
- Misses         13       14       +1     
Impacted Files Coverage Δ
server/user_account/urls.py 100.00% <ø> (ø)
server/user_account/views.py 93.40% <80.00%> (-0.85%) ⬇️
server/user_account/serializers.py 100.00% <100.00%> (ø)
server/user_account/tests.py 95.89% <100.00%> (+0.13%) ⬆️

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 7a71231...b02cade. Read the comment docs.

@github-actions
Copy link

Pylint Report

'+ ************* Module user_account.urls
'+ user_account/urls.py:18:2: W0511: TODO: Call mobile specific view (fixme)
'+ user_account/views.py:174:2: W0511: TODO: Implement mobile specific endpoint (fixme)
'- -----------------------------------
'- Your code has been rated at 9.95/10+ ------------------------------------------------------------------
'+ Your code has been rated at 9.91/10 (previous run: 9.95/10, -0.03)

@github-actions
Copy link

NG Lint Report

@makchamp makchamp marked this pull request as ready for review November 29, 2020 06:58
…tation error

This mobile specific login endpoint will be implemented later (when different options to password authentication are determined)
@github-actions
Copy link

Pylint Report

'+ ************* Module user_account.urls
'+ user_account/urls.py:18:2: W0511: TODO: Call mobile specific view (fixme)
'+ user_account/views.py:174:2: W0511: TODO: Implement mobile specific endpoint (fixme)
'- -----------------------------------
'- Your code has been rated at 9.95/10+ ------------------------------------------------------------------
'+ Your code has been rated at 9.91/10 (previous run: 9.95/10, -0.03)

@github-actions
Copy link

NG Lint Report

@github-actions
Copy link

Pylint Report

'+ ************* Module user_account.urls
'+ user_account/urls.py:18:2: W0511: TODO: Call mobile specific view (fixme)
'+ user_account/views.py:174:2: W0511: TODO: Implement mobile specific endpoint (fixme)
'- -----------------------------------
'- Your code has been rated at 9.95/10+ ------------------------------------------------------------------
'+ Your code has been rated at 9.92/10 (previous run: 9.95/10, -0.03)

@github-actions
Copy link

NG Lint Report

@github-actions
Copy link

Pylint Report

'+ ************* Module user_account.urls
'+ user_account/urls.py:18:2: W0511: TODO: Call mobile specific view (fixme)
'+ user_account/views.py:174:2: W0511: TODO: Implement mobile specific endpoint (fixme)
'- -----------------------------------
'- Your code has been rated at 9.95/10+ ------------------------------------------------------------------
'+ Your code has been rated at 9.92/10 (previous run: 9.95/10, -0.03)

@github-actions
Copy link

NG Lint Report

@sonarcloud
Copy link

sonarcloud bot commented Nov 30, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@NicholasNagy NicholasNagy left a comment

Choose a reason for hiding this comment

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

I tested everything out extensively, I don't see any issues feature wise, nor code wise.
I will merge it now.

@NicholasNagy NicholasNagy merged commit d642a47 into master Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants