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

[NRPTI-1045][FEATURE] - issuing agency update #1120

Merged
merged 80 commits into from
Oct 16, 2023

Conversation

sanjaytkbabu
Copy link
Contributor

@sanjaytkbabu sanjaytkbabu commented Oct 4, 2023

Pull Request Standards

  • The title of the PR is accurate
  • The title includes the type of change [HOTFIX, FEATURE, etc]
  • The PR title includes the ticket number in format of [NRPTI-###]
  • Documentation is updated to reflect change

Description

This PR includes the following proposed change(s):

  • { issuingAgency is now stored in database and retrieved via api calls in nrpti (all pages), nrced(all pages) and nrpti importers }
  • { Include any screenshots necessary }

davidclaveau and others added 30 commits September 11, 2023 13:53
…update values from that list with value from a text box.
…e the object which will be sent to the api for updating the agencyList. Also worth noting the alerts are being used here for console debugging. My local settings are preventing logging, when I find the reason I will update readme to document process
@davidclaveau davidclaveau changed the title Feature/1045 issuing agency update [NRPTI-1045] - issuing agency update Oct 4, 2023
@sanjaytkbabu sanjaytkbabu changed the title [NRPTI-1045] - issuing agency update [NRPTI-1045][FEATURE] - issuing agency update Oct 16, 2023
Copy link
Contributor

@LocalNewsTV LocalNewsTV left a comment

Choose a reason for hiding this comment

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

Feedback

  • Love the new JSDoc Headers, major improvement in the codebase from before.
  • nice work getting the updated changes between the two frontends from the existing implementation

We should use a constants object with all the agency codes in it, currently if I were to update the 'AGENCY_FNLR_NRO" alone, I would have to change that value 20 times across 11 files.

If we use a constants file for agency codes we can prevent issues that can occur from using a hard coded string in several places:

  • Typos
  • Reduce code complexity
  • Make maintenance faster and easier
  • Reduce potential bugs from future feature updates and the above

Currently there is a bug in the new features!

  • If I change the name of an agency it runs successfully ✅
  • If I change the name of an agency a second time on the same page, no network request is sent, a false confirmation message appears, values do not change.
  • If I enter an updated agency name, do not select an agency I want to update, I still receive a false positive from both the API and the frontend (Or just the frontend, if this is a subsequent request on the same page)

@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@LocalNewsTV LocalNewsTV self-requested a review October 16, 2023 21:43
@sanjaytkbabu sanjaytkbabu merged commit 97271a8 into master Oct 16, 2023
4 of 5 checks passed
@sanjaytkbabu sanjaytkbabu deleted the feature/1045-issuing-agency-update branch October 16, 2023 21:45
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.

4 participants